Fixes and tests

packages/backend-core/src/cache/docWritethrough.ts

@@ -23,6 +23,8 @@ export class DocWritethrough {
   private _docId: string
   private writeRateMs: number
 
+  private docInfoCacheKey: string
+
   constructor(
     db: Database,
     docId: string,
@@ -31,6 +33,7 @@ export class DocWritethrough {
     this.db = db
     this._docId = docId
     this.writeRateMs = writeRateMs
+    this.docInfoCacheKey = `${this.docId}:info`
   }
 
   get docId() {
@@ -44,25 +47,38 @@ export class DocWritethrough {
   async patch(data: Record<string, any>) {
     const cache = await getCache()
 
-    const key = `${this.docId}:info`
-    const cacheItem = await cache.withCache(
-      key,
-      null,
-      () => this.makeCacheItem(),
+    await this.storeToCache(cache, data)
+
+    const updateDb = await this.shouldUpdateDb(cache)
+
+    if (updateDb) {
+      const lockResponse = await locks.doWithLock(
         {
-        useTenancy: false,
+          type: LockType.TRY_ONCE,
+          name: LockName.PERSIST_WRITETHROUGH,
+          resource: this.docInfoCacheKey,
+          ttl: 15000,
+        },
+        async () => {
+          if (await this.shouldUpdateDb(cache)) {
+            await this.persistToDb(cache)
+            await cache.store(this.docInfoCacheKey, this.makeCacheItem())
+          }
         }
       )
 
-    await this.storeToCache(cache, data)
-
-    const updateDb =
-      !cacheItem || cacheItem.lastWrite <= Date.now() - this.writeRateMs
-    // let output = this.doc
-    if (updateDb) {
-      await this.persistToDb(cache)
+      if (!lockResponse.executed) {
+        console.log(`Ignoring redlock conflict in write-through cache`)
       }
     }
+  }
+
+  private async shouldUpdateDb(cache: BaseCache) {
+    const cacheItem = await cache.withCache(this.docInfoCacheKey, null, () =>
+      this.makeCacheItem()
+    )
+    return cacheItem.lastWrite <= Date.now() - this.writeRateMs
+  }
 
   private async storeToCache(cache: BaseCache, data: Record<string, any>) {
     for (const [key, value] of Object.entries(data)) {
@@ -72,16 +88,6 @@ export class DocWritethrough {
   }
 
   private async persistToDb(cache: BaseCache) {
-    const key = `${this.db.name}_${this.docId}`
-
-    const lockResponse = await locks.doWithLock(
-      {
-        type: LockType.TRY_ONCE,
-        name: LockName.PERSIST_WRITETHROUGH,
-        resource: key,
-        ttl: 15000,
-      },
-      async () => {
     let doc: AnyDocument | undefined
     try {
       doc = await this.db.get(this.docId)
@@ -101,10 +107,4 @@ export class DocWritethrough {
       await cache.delete(key, { useTenancy: false })
     }
   }
-    )
-
-    if (!lockResponse.executed) {
-      throw `DocWriteThrough could not be persisted to db for ${key}`
-    }
-  }
 }

packages/backend-core/src/cache/tests/docWritethrough.spec.ts

@@ -1,12 +1,10 @@
 import tk from "timekeeper"
-import { env } from "../.."
+
 import { DBTestConfiguration, generator, structures } from "../../../tests"
 import { getDB } from "../../db"
 import { DocWritethrough } from "../docWritethrough"
 import _ from "lodash"
 
-env._set("MOCK_REDIS", null)
-
 const WRITE_RATE_MS = 500
 
 const initialTime = Date.now()
@@ -238,5 +236,42 @@ describe("docWritethrough", () => {
         )
       })
     })
+
+    it("concurrent calls will not cause multiple saves", async () => {
+      async function parallelPatch(count: number) {
+        await Promise.all(
+          Array.from({ length: count }).map(() =>
+            docWritethrough.patch(generatePatchObject(1))
+          )
+        )
+      }
+
+      const persistToDbSpy = jest.spyOn(docWritethrough as any, "persistToDb")
+      const storeToCacheSpy = jest.spyOn(docWritethrough as any, "storeToCache")
+
+      await config.doInTenant(async () => {
+        await parallelPatch(5)
+        expect(persistToDbSpy).not.toBeCalled()
+        expect(storeToCacheSpy).toBeCalledTimes(5)
+
+        travelForward(WRITE_RATE_MS)
+
+        await parallelPatch(40)
+
+        expect(persistToDbSpy).toBeCalledTimes(1)
+        expect(storeToCacheSpy).toBeCalledTimes(45)
+
+        await parallelPatch(10)
+
+        expect(persistToDbSpy).toBeCalledTimes(1)
+        expect(storeToCacheSpy).toBeCalledTimes(55)
+
+        travelForward(WRITE_RATE_MS)
+
+        await parallelPatch(5)
+        expect(persistToDbSpy).toBeCalledTimes(2)
+        expect(storeToCacheSpy).toBeCalledTimes(60)
+      })
+    })
   })
 })