From 883c95ea6370402ea24345c884f9c24f23c4f077 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 24 Jan 2025 13:00:01 +0000 Subject: [PATCH] [project-history] block filestore reads from old queue entries (#23096) GitOrigin-RevId: 9952cb66e542b17a6a3b5e3b2609d53dc8c371fd --- .../app/js/HistoryStoreManager.js | 3 + .../config/settings.defaults.cjs | 1 + .../test/acceptance/js/SyncTests.js | 137 ++++++++++++++++++ .../HistoryStoreManagerTests.js | 25 ++++ 4 files changed, 166 insertions(+) diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 698c0618ac..24a70a19b1 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -363,6 +363,9 @@ export function createBlobForUpdate(projectId, historyId, update, callback) { new OError('no filestore URL provided and blob was not created') ) } + if (!Settings.apis.filestore.enabled) { + return callback(new OError('blocking filestore read', { update })) + } fetchStream(filestoreURL, { signal: AbortSignal.timeout(HTTP_REQUEST_TIMEOUT), diff --git a/services/project-history/config/settings.defaults.cjs b/services/project-history/config/settings.defaults.cjs index 15a96a8373..2338718203 100644 --- a/services/project-history/config/settings.defaults.cjs +++ b/services/project-history/config/settings.defaults.cjs @@ -27,6 +27,7 @@ module.exports = { url: `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016`, }, filestore: { + enabled: process.env.FILESTORE_ENABLED !== 'false', url: `http://${process.env.FILESTORE_HOST || '127.0.0.1'}:3009`, }, web: { diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index 8a72ff2a79..2f04ed24e6 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -5,6 +5,7 @@ import request from 'request' import assert from 'node:assert' import mongodb from 'mongodb-legacy' import logger from '@overleaf/logger' +import Settings from '@overleaf/settings' import * as ProjectHistoryClient from './helpers/ProjectHistoryClient.js' import * as ProjectHistoryApp from './helpers/ProjectHistoryApp.js' import sinon from 'sinon' @@ -621,6 +622,142 @@ describe('Syncing with web and doc-updater', function () { } ) }) + describe('with filestore disabled', function () { + before(function () { + Settings.apis.filestore.enabled = false + }) + after(function () { + Settings.apis.filestore.enabled = true + }) + it('should record error when blob is missing', function (done) { + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + persistedFile: { hash: EMPTY_FILE_HASH, byteLength: 0 }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }) + + const fileContents = Buffer.from([1, 2, 3]) + const fileHash = 'aed2973e4b8a7ff1b30ff5c4751e5a2b38989e74' + + MockFileStore() + .get(`/project/${this.project_id}/file/${this.file_id}`) + .reply(200, fileContents) + const headBlob = MockHistoryStore() + .head(`/api/projects/${historyId}/blobs/${fileHash}`) + .times(3) // three retries + .reply(404) + const createBlob = MockHistoryStore() + .put(`/api/projects/${historyId}/blobs/${fileHash}`, fileContents) + .reply(201) + + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + expect(body).to.deep.equal([ + { + v2Authors: [], + authors: [], + timestamp: this.timestamp.toJSON(), + operations: [ + { + pathname: 'test.png', + file: { + hash: fileHash, + }, + }, + ], + origin: { kind: 'test-origin' }, + }, + ]) + return true + }) + .query({ end_version: 0 }) + .reply(204) + + async.series( + [ + cb => { + ProjectHistoryClient.resyncHistory(this.project_id, cb) + }, + cb => { + const update = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [], + files: [ + { + file: this.file_id, + path: '/test.png', + _hash: fileHash, + url: `http://127.0.0.1:3009/project/${this.project_id}/file/${this.file_id}`, + }, + { path: '/persistedFile' }, + ], + }, + meta: { + ts: this.timestamp, + }, + } + ProjectHistoryClient.pushRawUpdate( + this.project_id, + update, + cb + ) + }, + cb => { + ProjectHistoryClient.flushProject( + this.project_id, + { + allowErrors: true, + }, + (err, res) => { + if (err) return cb(err) + assert( + res.statusCode === 500, + 'resync should have failed' + ) + cb() + } + ) + }, + ], + error => { + if (error) { + throw error + } + assert( + loggerError.calledWithMatch( + sinon.match.any, + 'blocking filestore read' + ), + 'error logged on 500' + ) + assert( + headBlob.isDone(), + 'HEAD /api/projects/:historyId/blobs/:hash should have been called' + ) + assert( + !createBlob.isDone(), + '/api/projects/:historyId/blobs/:hash should have been skipped' + ) + assert( + !addFile.isDone(), + `/api/projects/${historyId}/changes should have been skipped` + ) + done() + } + ) + }) + }) }) describe("when a file exists which shouldn't", function () { diff --git a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js index 3edf4e1aa7..db5b87d65c 100644 --- a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js +++ b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js @@ -22,6 +22,7 @@ describe('HistoryStoreManager', function () { }, apis: { filestore: { + enabled: true, url: 'http://filestore.overleaf.production', }, }, @@ -424,6 +425,30 @@ describe('HistoryStoreManager', function () { }) }) + describe('with filestore disabled', function () { + beforeEach(function (done) { + this.settings.apis.filestore.enabled = false + this.file_id = '012345678901234567890123' + this.update = { + file: true, + url: `http://filestore.other.cloud.provider/project/${this.projectId}/file/${this.file_id}`, + hash: this.hash, + } + this.HistoryStoreManager.createBlobForUpdate( + this.projectId, + this.historyId, + this.update, + err => { + expect(err).to.match(/blocking filestore read/) + done() + } + ) + }) + it('should not request the file', function () { + expect(this.FetchUtils.fetchStream).to.not.have.been.called + }) + }) + describe('for a file update with an invalid filestore location', function () { beforeEach(function (done) { this.invalid_id = '000000000000000000000000'