diff --git a/services/project-history/app/js/Errors.js b/services/project-history/app/js/Errors.js index d2052d0169..2365a538d9 100644 --- a/services/project-history/app/js/Errors.js +++ b/services/project-history/app/js/Errors.js @@ -11,3 +11,4 @@ export class UpdateWithUnknownFormatError extends OError {} export class UnexpectedOpTypeError extends OError {} export class TooManyRequestsError extends OError {} export class NeedFullProjectStructureResyncError extends OError {} +export class FileContentEmptyError extends OError {} diff --git a/services/project-history/app/js/HttpController.js b/services/project-history/app/js/HttpController.js index 57d32cee66..c9deb47393 100644 --- a/services/project-history/app/js/HttpController.js +++ b/services/project-history/app/js/HttpController.js @@ -645,9 +645,11 @@ const resyncProjectSchema = z.object({ }), query: z.object({ force: z.stringbool().default(false), + recoverCorruptedFiles: z.stringbool().default(false), }), body: z.object({ force: z.boolean().default(false), + recoverCorruptedFiles: z.boolean().default(false), origin: z .object({ kind: z.string(), @@ -668,6 +670,9 @@ export function resyncProject(req, res, next) { options.historyRangesMigration = body.historyRangesMigration } if (query.force || body.force) { + if (query.recoverCorruptedFiles || body.recoverCorruptedFiles) { + options.recoverCorruptedFiles = true + } // this will delete the queue and clear the sync state // use if the project is completely broken SyncManager.startHardResync(projectId, options, error => { diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 1443d6436a..9a083bf9d2 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -7,8 +7,13 @@ import Settings from '@overleaf/settings' import logger from '@overleaf/logger' import Metrics from '@overleaf/metrics' import OError from '@overleaf/o-error' -import { File, Range } from 'overleaf-editor-core' +import { File, Range, TextOperation } from 'overleaf-editor-core' import { + TooLongError, + UnprocessableError, +} from 'overleaf-editor-core/lib/errors.js' +import { + FileContentEmptyError, NeedFullProjectStructureResyncError, SYNC_ONGOING_ERROR_MESSAGE, SyncError, @@ -81,7 +86,7 @@ async function startHardResync(projectId, options = {}) { await clearResyncState(projectId) await RedisManager.promises.clearFirstOpTimestamp(projectId) await RedisManager.promises.destroyDocUpdatesQueue(projectId) - await startResyncWithoutLock(projectId, options) + await startResyncWithoutLock(projectId, { ...options, hard: true }) } ) } catch (error) { @@ -138,6 +143,8 @@ async function startResyncWithoutLock(projectId, options) { } syncState.setOrigin(options.origin || { kind: 'history-resync' }) syncState.startProjectStructureSync() + syncState.hardResync = options.hard === true + syncState.recoverCorruptedFiles = options.recoverCorruptedFiles === true const webOpts = {} if (options.historyRangesMigration) { @@ -345,7 +352,9 @@ async function expandSyncUpdates( const expander = new SyncUpdateExpander( projectId, snapshotFiles, - syncState.origin + syncState.origin, + syncState.hardResync, + syncState.recoverCorruptedFiles ) // expand updates asynchronously to avoid blocking @@ -369,7 +378,9 @@ class SyncState { history, stuckClearCount, lastStuckClearAt, - lastStuckDocPaths + lastStuckDocPaths, + hardResync, + recoverCorruptedFiles ) { this.projectId = projectId this.resyncProjectStructure = resyncProjectStructure @@ -382,6 +393,8 @@ class SyncState { this.stuckClearCount = stuckClearCount this.lastStuckClearAt = lastStuckClearAt this.lastStuckDocPaths = lastStuckDocPaths + this.hardResync = hardResync || false + this.recoverCorruptedFiles = recoverCorruptedFiles || false } static fromRaw(projectId, rawSyncState) { @@ -414,6 +427,8 @@ class SyncState { const stuckClearCount = rawSyncState.stuckClearCount ?? 0 const lastStuckClearAt = rawSyncState.lastStuckClearAt const lastStuckDocPaths = rawSyncState.lastStuckDocPaths + const hardResync = rawSyncState.hardResync || false + const recoverCorruptedFiles = rawSyncState.recoverCorruptedFiles || false return new SyncState( projectId, resyncProjectStructure, @@ -425,7 +440,9 @@ class SyncState { history, stuckClearCount, lastStuckClearAt, - lastStuckDocPaths + lastStuckDocPaths, + hardResync, + recoverCorruptedFiles ) } @@ -434,6 +451,8 @@ class SyncState { resyncProjectStructure: this.resyncProjectStructure, resyncDocContents: Array.from(this.resyncDocContents), origin: this.origin, + hardResync: this.hardResync, + recoverCorruptedFiles: this.recoverCorruptedFiles, } } @@ -562,13 +581,23 @@ class SyncUpdateExpander { * * @param {string} projectId * @param {Record} snapshotFiles - * @param {string} origin + * @param {import('overleaf-editor-core/lib/types.js').RawOrigin} origin + * @param {boolean} hardResync + * @param {boolean} recoverCorruptedFiles */ - constructor(projectId, snapshotFiles, origin) { + constructor( + projectId, + snapshotFiles, + origin, + hardResync, + recoverCorruptedFiles + ) { this.projectId = projectId this.files = snapshotFiles this.expandedUpdates = /** @type ProjectStructureUpdate[] */ [] this.origin = origin + this.hardResync = hardResync || false + this.recoverCorruptedFiles = recoverCorruptedFiles || false } // If there's an expected *file* with the same path and either the same hash @@ -888,18 +917,81 @@ class SyncUpdateExpander { // compute the difference between the expected and persisted content const historyId = await WebApiManager.promises.getHistoryId(this.projectId) - const file = await snapshotFile.load( - 'eager', - HistoryStoreManager.getBlobStore(historyId) - ) - const persistedContent = file.getContent() - if (persistedContent == null) { - // This should not happen given that we loaded the file eagerly. We could - // probably refine the types in overleaf-editor-core so that this check - // wouldn't be necessary. - throw new Error('File was not properly loaded') + + let file + try { + file = await snapshotFile.load( + 'eager', + HistoryStoreManager.getBlobStore(historyId) + ) + const persistedContent = file.getContent() + if (persistedContent == null) { + throw new FileContentEmptyError('File was not properly loaded') + } + } catch (err) { + // When the recoverCorruptedFiles flag is set (requires hard resync), + // recover from known data corruption errors by removing the file and + // re-adding it from docstore. For soft resyncs, hard resyncs without + // the flag, or transient errors, re-throw so the operation can be retried. + // Also bail out if the expected content exceeds the max string length, + // as the re-add would fail when applying text operations. This check + // must happen here (not in isDataCorruptionError) because the original + // error may be a different corruption type — excluding TooLongError + // from detection would cause the file to be removed but not re-added. + if (!this.recoverCorruptedFiles || !isDataCorruptionError(err)) { + logger.error({ + name: 'failed to load file from history during resync', + projectId: this.projectId, + pathname, + err, + }) + throw err + } + if (expectedContent.length > TextOperation.MAX_STRING_LENGTH) { + throw new TooLongError(null, expectedContent.length) + .withInfo({ + projectId: this.projectId, + pathname, + maxLength: TextOperation.MAX_STRING_LENGTH, + }) + .withCause(err) + } + + logger.warn( + { projectId: this.projectId, pathname, err }, + 'failed to load file from history during hard resync, removing and re-adding from docstore' + ) + Metrics.inc('project_history_resync_operation', 1, { + status: 'recover corrupted file', + }) + + this.expandedUpdates.push({ + pathname, + new_pathname: '', + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + }, + }) + this.expandedUpdates.push({ + pathname, + doc: update.doc, + docLines: expectedContent, + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + }, + }) + // Replace in-memory file so the range sync below diffs the empty + // persisted state against docstore ranges and restores them. + this.files[pathname] = File.fromString(expectedContent) + file = this.files[pathname] } + const persistedContent = /** @type {string} */ (file.getContent()) + if (!hashesMatch) { const expandedUpdate = await this.queueUpdateForOutOfSyncContent( update, @@ -1439,6 +1531,44 @@ function trackingDirectivesEqual(a, b) { } } +/** + * Determines whether an error from loading a file's blob indicates data + * corruption (safe to recover from by removing and re-adding the file) as + * opposed to a transient infrastructure failure (which should be retried). + * + * Known corruption indicators: + * - UnprocessableError (and subclasses ApplyError, InvalidInsertionError, + * TooLongError): the stored operations are inconsistent with the blob content + * - SyntaxError: the ranges blob contains invalid JSON + * - FileContentEmptyError: blob loaded but returned null content + * + * @param {unknown} err + * @returns {boolean} + */ +function isDataCorruptionError(err) { + if (!(err instanceof Error)) { + return false + } + + // Operation apply failures (op/content mismatch). This covers ApplyError, + // InvalidInsertionError, and TooLongError from overleaf-editor-core. + if (err instanceof UnprocessableError) { + return true + } + + // Corrupted ranges blob (invalid JSON from BlobStore.getObject) + if (err instanceof SyntaxError) { + return true + } + + // Null content after loading + if (err instanceof FileContentEmptyError) { + return true + } + + return false +} + // EXPORTS const cloneResyncStateCb = callbackify(cloneResyncState) diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index 108f988b2a..37f9e490e8 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -169,7 +169,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) }) @@ -263,7 +263,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) it('should skip HEAD on blob without hash', async function () { @@ -353,7 +353,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) it('should record error when checking blob fails with 500', async function () { @@ -453,7 +453,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( !addFile.isDone(), - `/api/projects/${historyId}/changes should have been skipped` + `/api/projects/${historyId}/legacy_changes should have been skipped` ) }) it('should skip blob write when blob exists', async function () { @@ -544,7 +544,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) it('should add file w/o url', async function () { @@ -635,7 +635,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) describe('with filestore disabled', function () { @@ -746,7 +746,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( !addFile.isDone(), - `/api/projects/${historyId}/changes should have been skipped` + `/api/projects/${historyId}/legacy_changes should have been skipped` ) }) }) @@ -852,7 +852,7 @@ describe('Syncing with web and doc-updater', function () { ) assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) }) @@ -921,7 +921,7 @@ describe('Syncing with web and doc-updater', function () { assert( deleteFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) }) @@ -1008,7 +1008,7 @@ describe('Syncing with web and doc-updater', function () { assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) const project = await db.projects.findOne({ @@ -1073,7 +1073,7 @@ describe('Syncing with web and doc-updater', function () { assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1150,7 +1150,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1222,7 +1222,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1324,7 +1324,7 @@ describe('Syncing with web and doc-updater', function () { assert( fixTrackedChange.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1419,11 +1419,360 @@ describe('Syncing with web and doc-updater', function () { assert( fixTrackedChange.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) }) + describe("when a doc's blob content is corrupted", function () { + beforeEach(function () { + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + 'main.tex': { + hash: '0a207c060e61f3b88eaee0a8cd0696f46fb155eb', + stringLength: 10, + }, + }, + }, + changes: [ + { + operations: [ + { + pathname: 'main.tex', + // Retain 10 chars, but blob content is only 3 chars. + // This simulates a corrupted history where the + // operation doesn't match the blob content, causing + // an ApplyError when the file is loaded eagerly. + textOperation: [10], + }, + ], + timestamp: '2026-01-01T00:00:00.000Z', + authors: [], + v2Authors: [], + }, + ], + }, + startVersion: 0, + }, + }) + + // Blob returns valid content, but it doesn't match the operation + MockHistoryStore() + .get( + `/api/projects/${historyId}/blobs/0a207c060e61f3b88eaee0a8cd0696f46fb155eb` + ) + .reply(200, 'a\nb') + }) + + it('should remove and re-add the file during hard resync', async function () { + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + // The corrupted file is removed and re-added as two changes + expect(body).to.have.length(2) + // First change: remove the corrupted file + expect(body[0].operations).to.have.length(1) + expect(body[0].operations[0]).to.deep.include({ + pathname: 'main.tex', + newPathname: '', + }) + // Second change: re-add with new blob from docstore + expect(body[1].operations).to.have.length(1) + expect(body[1].operations[0].pathname).to.equal('main.tex') + expect(body[1].operations[0].file).to.have.property('hash') + return true + }) + .query({ end_version: 1 }) + .reply(204) + + // The re-added file needs its blob to be stored + MockHistoryStore() + .put(new RegExp(`/api/projects/${historyId}/blobs/`)) + .reply(201) + + await ProjectHistoryClient.hardResyncHistory(this.project_id, { + recoverCorruptedFiles: true, + }) + + const update1 = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update1) + + const update2 = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'hello world', + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update2) + + await ProjectHistoryClient.flushProject(this.project_id) + + assert( + addFile.isDone(), + `/api/projects/${historyId}/legacy_changes should have been called` + ) + sinon.assert.calledWithMatch( + loggerWarn, + sinon.match({ + projectId: this.project_id, + pathname: 'main.tex', + }), + 'failed to load file from history during hard resync, removing and re-adding from docstore' + ) + }) + + it('should restore comments and tracked changes from docstore after hard resync re-add', async function () { + const commentId = 'comment-id-1' + + const addFile = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + // Expect three changes: remove, re-add, then comment sync + expect(body).to.have.length(3) + // First change: remove the corrupted file + expect(body[0].operations).to.have.length(1) + expect(body[0].operations[0]).to.deep.include({ + pathname: 'main.tex', + newPathname: '', + }) + // Second change: re-add with new blob from docstore + expect(body[1].operations).to.have.length(1) + expect(body[1].operations[0].pathname).to.equal('main.tex') + expect(body[1].operations[0].file).to.have.property('hash') + // Third change: restore comment from docstore ranges + expect(body[2].operations).to.have.length(1) + expect(body[2].operations[0]).to.deep.include({ + pathname: 'main.tex', + commentId, + }) + expect(body[2].operations[0].ranges).to.deep.equal([ + { pos: 0, length: 5 }, + ]) + return true + }) + .query({ end_version: 1 }) + .reply(204) + + // The re-added file needs its blob to be stored + MockHistoryStore() + .put(new RegExp(`/api/projects/${historyId}/blobs/`)) + .reply(201) + + await ProjectHistoryClient.hardResyncHistory(this.project_id, { + recoverCorruptedFiles: true, + }) + + const update1 = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update1) + + const update2 = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'hello world', + ranges: { + comments: [ + { + id: commentId, + op: { + c: 'hello', + p: 0, + hpos: 0, + hlen: 5, + t: commentId, + }, + meta: { + user_id: 'user-id', + ts: this.timestamp, + }, + }, + ], + changes: [], + }, + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update2) + + await ProjectHistoryClient.flushProject(this.project_id) + + assert( + addFile.isDone(), + `/api/projects/${historyId}/legacy_changes should have been called` + ) + }) + + it('should propagate the error during soft resync', async function () { + await ProjectHistoryClient.resyncHistory(this.project_id) + + const update1 = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update1) + + const update2 = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'hello world', + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update2) + + // Soft resync should fail — the error is propagated so it can be + // retried later (the blob store may be temporarily unavailable). + const { statusCode } = await ProjectHistoryClient.flushProject( + this.project_id, + { allowErrors: true } + ) + expect(statusCode).to.equal(500) + }) + + it('should propagate the error during hard resync without recoverCorruptedFiles flag', async function () { + await ProjectHistoryClient.hardResyncHistory(this.project_id) + + const update1 = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update1) + + const update2 = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'hello world', + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update2) + + // Hard resync without the recoverCorruptedFiles flag should not + // recover — recovery is too destructive to be the default behaviour. + const { statusCode } = await ProjectHistoryClient.flushProject( + this.project_id, + { allowErrors: true } + ) + expect(statusCode).to.equal(500) + }) + }) + + describe("when a doc's blob store is temporarily unavailable", function () { + beforeEach(function () { + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + 'main.tex': { + hash: '0a207c060e61f3b88eaee0a8cd0696f46fb155eb', + stringLength: 3, + }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }) + + // Blob returns a 500 simulating a transient server error + MockHistoryStore() + .get( + `/api/projects/${historyId}/blobs/0a207c060e61f3b88eaee0a8cd0696f46fb155eb` + ) + .reply(500, 'Internal Server Error') + }) + + it('should propagate transient errors even during hard resync', async function () { + await ProjectHistoryClient.hardResyncHistory(this.project_id) + + const update1 = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update1) + + const update2 = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'hello world', + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + await ProjectHistoryClient.pushRawUpdate(this.project_id, update2) + + // Even hard resync should not recover from transient errors (5xx). + // The error should propagate so it can be retried. + const { statusCode } = await ProjectHistoryClient.flushProject( + this.project_id, + { allowErrors: true } + ) + expect(statusCode).to.equal(500) + }) + }) + describe("when a doc's ranges are out of sync", function () { const commentId = 'comment-id' beforeEach(function () { @@ -1580,7 +1929,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1682,7 +2031,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1771,7 +2120,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1861,7 +2210,7 @@ describe('Syncing with web and doc-updater', function () { assert( addComment.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -1977,7 +2326,7 @@ describe('Syncing with web and doc-updater', function () { assert( fixTrackedChange.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -2081,7 +2430,7 @@ describe('Syncing with web and doc-updater', function () { assert( fixTrackedChange.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) @@ -2199,7 +2548,7 @@ describe('Syncing with web and doc-updater', function () { assert( fixTrackedChange.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) }) }) @@ -2292,7 +2641,7 @@ describe('Syncing with web and doc-updater', function () { assert( addFile.isDone(), - `/api/projects/${historyId}/changes should have been called` + `/api/projects/${historyId}/legacy_changes should have been called` ) assert( !docContentRequest.isDone(), diff --git a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js index 6d221bed3c..f3e87b59a3 100644 --- a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js +++ b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js @@ -164,6 +164,24 @@ export async function resyncHistory(projectId) { expect(response.status).to.equal(204) } +export async function hardResyncHistory( + projectId, + { recoverCorruptedFiles } = {} +) { + const params = new URLSearchParams({ force: 'true' }) + if (recoverCorruptedFiles) { + params.set('recoverCorruptedFiles', 'true') + } + const response = await fetchNothing( + `http://127.0.0.1:3054/project/${projectId}/resync?${params}`, + { + method: 'POST', + json: { origin: { kind: 'test-origin' } }, + } + ) + expect(response.status).to.equal(204) +} + export async function createLabel( projectId, userId, diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index a89325fe03..897d70492a 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -3,7 +3,9 @@ import { expect } from 'chai' import mongodb from 'mongodb-legacy' import tk from 'timekeeper' import { File, Comment, TrackedChange, Range } from 'overleaf-editor-core' +import { UnprocessableError } from 'overleaf-editor-core/lib/errors.js' import { strict as esmock } from 'esmock' +import { FileContentEmptyError } from '../../../../app/js/Errors.js' const { ObjectId } = mongodb const MODULE_PATH = '../../../../app/js/SyncManager.js' @@ -144,20 +146,29 @@ describe('SyncManager', function () { }, } - this.SyncManager = await esmock(MODULE_PATH, { - '../../../../app/js/LockManager.js': this.LockManager, - '../../../../app/js/UpdateCompressor.js': this.UpdateCompressor, - '../../../../app/js/UpdateTranslator.js': this.UpdateTranslator, - '../../../../app/js/mongodb.js': { ObjectId, db: this.db }, - '../../../../app/js/WebApiManager.js': this.WebApiManager, - '../../../../app/js/ErrorRecorder.js': this.ErrorRecorder, - '../../../../app/js/RedisManager.js': this.RedisManager, - '../../../../app/js/SnapshotManager.js': this.SnapshotManager, - '../../../../app/js/HistoryStoreManager.js': this.HistoryStoreManager, - '../../../../app/js/HashManager.js': this.HashManager, - '@overleaf/metrics': this.Metrics, - '@overleaf/settings': this.Settings, - }) + this.SyncManager = await esmock( + MODULE_PATH, + { + '../../../../app/js/LockManager.js': this.LockManager, + '../../../../app/js/UpdateCompressor.js': this.UpdateCompressor, + '../../../../app/js/UpdateTranslator.js': this.UpdateTranslator, + '../../../../app/js/mongodb.js': { ObjectId, db: this.db }, + '../../../../app/js/WebApiManager.js': this.WebApiManager, + '../../../../app/js/ErrorRecorder.js': this.ErrorRecorder, + '../../../../app/js/RedisManager.js': this.RedisManager, + '../../../../app/js/SnapshotManager.js': this.SnapshotManager, + '../../../../app/js/HistoryStoreManager.js': this.HistoryStoreManager, + '../../../../app/js/HashManager.js': this.HashManager, + '@overleaf/metrics': this.Metrics, + '@overleaf/settings': this.Settings, + }, + { + 'overleaf-editor-core/lib/errors.js': + await import('overleaf-editor-core/lib/errors.js'), + '../../../../app/js/Errors.js': + await import('../../../../app/js/Errors.js'), + } + ) }) afterEach(function () { @@ -533,6 +544,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: ['new.tex'], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) @@ -549,6 +562,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: ['new.tex'], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) @@ -566,6 +581,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: [], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) @@ -585,6 +602,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: ['new.tex'], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) @@ -608,6 +627,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: [], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) @@ -632,6 +653,8 @@ describe('SyncManager', function () { resyncProjectStructure: false, resyncDocContents: [], origin: { kind: 'history-resync' }, + hardResync: false, + recoverCorruptedFiles: false, }) }) }) @@ -1571,6 +1594,116 @@ describe('SyncManager', function () { expect(this.extendLock).to.have.been.called }) }) + + describe('when the blob content is corrupted during hard resync', function () { + describe('and the recoverCorruptedFiles flag is true', function () { + beforeEach(function () { + this.syncState.recoverCorruptedFiles = true + }) + + async function assertRecovery(ctx, err) { + ctx.fileMap['main.tex'].load.rejects(err) + const updates = [ + resyncProjectStructureUpdate( + [ctx.persistedDoc], + [ctx.persistedFile] + ), + docContentSyncUpdate(ctx.persistedDoc, ctx.persistedDocContent), + ] + const expandedUpdates = + await ctx.SyncManager.promises.expandSyncUpdates( + ctx.projectId, + ctx.historyId, + ctx.mostRecentChunk, + updates, + ctx.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + pathname: 'main.tex', + new_pathname: '', + meta: { + resync: true, + origin: { kind: 'history-resync' }, + ts: TIMESTAMP, + }, + }, + { + pathname: 'main.tex', + doc: ctx.persistedDoc.doc, + docLines: ctx.persistedDocContent, + meta: { + resync: true, + origin: { kind: 'history-resync' }, + ts: TIMESTAMP, + }, + }, + ]) + } + + it('queues REMOVE + ADD for UnprocessableError', async function () { + await assertRecovery(this, new UnprocessableError('apply failed')) + }) + + it('queues REMOVE + ADD for SyntaxError', async function () { + await assertRecovery(this, new SyntaxError('bad JSON')) + }) + + it('queues REMOVE + ADD for FileContentEmptyError', async function () { + await assertRecovery( + this, + new FileContentEmptyError('null content') + ) + }) + + it('propagates non-corruption errors during hard resync', async function () { + this.fileMap['main.tex'].load.rejects( + new Error('connection timeout') + ) + const updates = [ + resyncProjectStructureUpdate( + [this.persistedDoc], + [this.persistedFile] + ), + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent), + ] + await expect( + this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + this.mostRecentChunk, + updates, + this.extendLock + ) + ).to.be.rejectedWith('connection timeout') + }) + }) + + describe('and the recoverCorruptedFiles flag is false', function () { + it('propagates corruption errors', async function () { + this.syncState.recoverCorruptedFiles = false + this.fileMap['main.tex'].load.rejects( + new UnprocessableError('apply failed') + ) + const updates = [ + resyncProjectStructureUpdate( + [this.persistedDoc], + [this.persistedFile] + ), + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent), + ] + await expect( + this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + this.mostRecentChunk, + updates, + this.extendLock + ) + ).to.be.rejectedWith('apply failed') + }) + }) + }) }) describe('syncing comments', function () {