From a7466a7291829908bc9ca16cda0e5d15ec9b20e2 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 24 Apr 2025 09:04:17 +0100 Subject: [PATCH] Merge pull request #24966 from overleaf/bg-history-buffer-optimised-get add getCurrentChunkIfValid function GitOrigin-RevId: e947a99ac928b58048a87cea0be1da34fcf3a9f8 --- .../storage/lib/chunk_buffer/index.js | 7 +- .../storage/lib/chunk_store/redis.js | 57 ++++++++ .../storage/chunk_store_redis_backend.test.js | 128 ++++++++++++++++++ 3 files changed, 188 insertions(+), 4 deletions(-) diff --git a/services/history-v1/storage/lib/chunk_buffer/index.js b/services/history-v1/storage/lib/chunk_buffer/index.js index fe30b993d7..5ef533ddba 100644 --- a/services/history-v1/storage/lib/chunk_buffer/index.js +++ b/services/history-v1/storage/lib/chunk_buffer/index.js @@ -14,13 +14,12 @@ const metrics = require('@overleaf/metrics') * @return {Promise.} */ async function loadLatest(projectId) { - const cachedChunk = await redisBackend.getCurrentChunk(projectId) const chunkRecord = await chunkStore.loadLatestRaw(projectId) - const cachedChunkIsValid = redisBackend.checkCacheValidityWithMetadata( - cachedChunk, + const cachedChunk = await redisBackend.getCurrentChunkIfValid( + projectId, chunkRecord ) - if (cachedChunkIsValid) { + if (cachedChunk) { metrics.inc('chunk_buffer.loadLatest', 1, { status: 'cache-hit', }) diff --git a/services/history-v1/storage/lib/chunk_store/redis.js b/services/history-v1/storage/lib/chunk_store/redis.js index 55f36ffa3b..7e542a2b01 100644 --- a/services/history-v1/storage/lib/chunk_store/redis.js +++ b/services/history-v1/storage/lib/chunk_store/redis.js @@ -62,6 +62,62 @@ async function getCurrentChunk(projectId) { } } +rclient.defineCommand('get_current_chunk_if_valid', { + numberOfKeys: 3, + lua: ` + local expectedStartVersion = ARGV[1] + local expectedChangesCount = tonumber(ARGV[2]) + local startVersionValue = redis.call('GET', KEYS[2]) + if not startVersionValue then + return nil -- this is a cache-miss + end + if startVersionValue ~= expectedStartVersion then + return nil -- this is a cache-miss + end + local changesCount = redis.call('LLEN', KEYS[3]) + if changesCount ~= expectedChangesCount then + return nil -- this is a cache-miss + end + local snapshotValue = redis.call('GET', KEYS[1]) + local changesValues = redis.call('LRANGE', KEYS[3], 0, -1) + return {snapshotValue, startVersionValue, changesValues} + `, +}) + +async function getCurrentChunkIfValid(projectId, chunkRecord) { + try { + const changesCount = chunkRecord.endVersion - chunkRecord.startVersion + const result = await rclient.get_current_chunk_if_valid( + keySchema.snapshot({ projectId }), + keySchema.startVersion({ projectId }), + keySchema.changes({ projectId }), + chunkRecord.startVersion, + changesCount + ) + if (!result) { + return null // cache-miss + } + const snapshot = Snapshot.fromRaw(JSON.parse(result[0])) + const startVersion = parseInt(result[1], 10) + const changes = result[2].map(c => Change.fromRaw(JSON.parse(c))) + const history = new History(snapshot, changes) + const chunk = new Chunk(history, startVersion) + metrics.inc('chunk_store.redis.get_current_chunk_if_valid', 1, { + status: 'success', + }) + return chunk + } catch (err) { + logger.error( + { err, projectId, chunkRecord }, + 'error getting current chunk from redis' + ) + metrics.inc('chunk_store.redis.get_current_chunk_if_valid', 1, { + status: 'error', + }) + return null + } +} + rclient.defineCommand('get_current_chunk_metadata', { numberOfKeys: 2, lua: ` @@ -245,6 +301,7 @@ async function clearCache(projectId) { module.exports = { getCurrentChunk, + getCurrentChunkIfValid, setCurrentChunk, getCurrentChunkMetadata, checkCacheValidity, diff --git a/services/history-v1/test/acceptance/js/storage/chunk_store_redis_backend.test.js b/services/history-v1/test/acceptance/js/storage/chunk_store_redis_backend.test.js index a4ce4f9653..4195632b50 100644 --- a/services/history-v1/test/acceptance/js/storage/chunk_store_redis_backend.test.js +++ b/services/history-v1/test/acceptance/js/storage/chunk_store_redis_backend.test.js @@ -603,4 +603,132 @@ describe('chunk store Redis backend', function () { expect(chunkMetadata.changesCount).to.equal(3) }) }) + + describe('getCurrentChunkIfValid', function () { + it('should return the chunk when versions and changes count match', async function () { + // Create and cache a sample chunk + const snapshot = new Snapshot() + const changes = [ + new Change( + [new AddFileOperation('test.tex', File.fromString('Valid content'))], + new Date(), + [] + ), + ] + const history = new History(snapshot, changes) + const chunk = new Chunk(history, 7) // startVersion 7, endVersion 8 + await redisBackend.setCurrentChunk(projectId, chunk) + + // Prepare chunkRecord matching the cached chunk + const chunkRecord = { startVersion: 7, endVersion: 8 } + + // Retrieve using getCurrentChunkIfValid + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + + expect(validChunk).to.not.be.null + expect(validChunk.getStartVersion()).to.equal(7) + expect(validChunk.getEndVersion()).to.equal(8) + expect(validChunk).to.deep.equal(chunk) + }) + + it('should return null when no chunk is cached', async function () { + // No chunk is cached for this projectId yet + const chunkRecord = { startVersion: 1, endVersion: 2 } + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + expect(validChunk).to.be.null + }) + + it('should return null when start version mismatches', async function () { + // Cache a chunk with startVersion 10 + const snapshot = new Snapshot() + const changes = [ + new Change( + [new AddFileOperation('test.tex', File.fromString('Content'))], + new Date(), + [] + ), + ] + const history = new History(snapshot, changes) + const chunk = new Chunk(history, 10) // startVersion 10, endVersion 11 + await redisBackend.setCurrentChunk(projectId, chunk) + + // Attempt to retrieve with a different startVersion + const chunkRecord = { startVersion: 9, endVersion: 10 } // Incorrect startVersion + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + expect(validChunk).to.be.null + }) + + it('should return null when changes count mismatches', async function () { + // Cache a chunk with one change (startVersion 15, endVersion 16) + const snapshot = new Snapshot() + const changes = [ + new Change( + [new AddFileOperation('test.tex', File.fromString('Content'))], + new Date(), + [] + ), + ] + const history = new History(snapshot, changes) + const chunk = new Chunk(history, 15) + await redisBackend.setCurrentChunk(projectId, chunk) + + // Attempt to retrieve with correct startVersion but incorrect endVersion (implying wrong changes count) + const chunkRecord = { startVersion: 15, endVersion: 17 } // Incorrect endVersion (implies 2 changes) + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + expect(validChunk).to.be.null + }) + + it('should return the chunk when versions and changes count match for a zero-change chunk', async function () { + // Cache a chunk with zero changes + const snapshot = new Snapshot() + const changes = [] + const history = new History(snapshot, changes) + const chunk = new Chunk(history, 20) // startVersion 20, endVersion 20 + await redisBackend.setCurrentChunk(projectId, chunk) + + // Prepare chunkRecord matching the zero-change chunk + const chunkRecord = { startVersion: 20, endVersion: 20 } + + // Retrieve using getCurrentChunkIfValid + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + + expect(validChunk).to.not.be.null + expect(validChunk.getStartVersion()).to.equal(20) + expect(validChunk.getEndVersion()).to.equal(20) + expect(validChunk.history.changes.length).to.equal(0) + expect(validChunk).to.deep.equal(chunk) + }) + + it('should return null when start version matches but changes count is wrong for zero-change chunk', async function () { + // Cache a chunk with zero changes + const snapshot = new Snapshot() + const changes = [] + const history = new History(snapshot, changes) + const chunk = new Chunk(history, 25) // startVersion 25, endVersion 25 + await redisBackend.setCurrentChunk(projectId, chunk) + + // Attempt to retrieve with correct startVersion but incorrect endVersion + const chunkRecord = { startVersion: 25, endVersion: 26 } // Incorrect endVersion (implies 1 change) + const validChunk = await redisBackend.getCurrentChunkIfValid( + projectId, + chunkRecord + ) + expect(validChunk).to.be.null + }) + }) })