From cae558af69b95c9a4124a91af44e3f32714f5cba Mon Sep 17 00:00:00 2001 From: Mathias Jakobsen Date: Thu, 16 Apr 2026 11:51:44 +0100 Subject: [PATCH] Merge pull request #32779 from overleaf/mj-project-history-bug [project-history] Fix broken ranges from history resync GitOrigin-RevId: c839e1d187768b7ce3411d06f78f017ebf7fec6e --- .../app/js/UpdateCompressor.js | 9 + .../test/acceptance/js/SyncTests.js | 171 ++++++++++++++++++ .../UpdateCompressor/UpdateCompressorTests.js | 92 ++++++++++ 3 files changed, 272 insertions(+) diff --git a/services/project-history/app/js/UpdateCompressor.js b/services/project-history/app/js/UpdateCompressor.js index 166a728cde..43beaedf82 100644 --- a/services/project-history/app/js/UpdateCompressor.js +++ b/services/project-history/app/js/UpdateCompressor.js @@ -314,6 +314,15 @@ function _concatTwoUpdates(firstUpdate, secondUpdate) { return [firstUpdate, secondUpdate] } + if (firstUpdate.meta.resync || secondUpdate.meta.resync) { + // Do not merge ops where one of them is a resync. We produce a list of + // resync ops that first corrects the content, then the ranges. By + // compressing the content updates seperately from the ranges updates, + // the ranges can become out-of-sync. To stop this, disallow compressing + // any resync updates. + return [firstUpdate, secondUpdate] + } + if (firstUpdate.meta.user_id !== secondUpdate.meta.user_id) { return [firstUpdate, secondUpdate] } diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index 563ac6e7c4..28e489cacf 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -4,11 +4,19 @@ import assert from 'node:assert' import mongodb from 'mongodb-legacy' import logger from '@overleaf/logger' import Settings from '@overleaf/settings' +import { + Snapshot, + File, + FileMap, + StringFileData, + Change, +} from 'overleaf-editor-core' import * as ProjectHistoryClient from './helpers/ProjectHistoryClient.js' import * as ProjectHistoryApp from './helpers/ProjectHistoryApp.js' import sinon from 'sinon' import { getFailure } from './helpers/ProjectHistoryClient.js' import { fetchNothing, RequestFailedError } from '@overleaf/fetch-utils' +import { _getBlobHashFromString } from '../../../app/js/HashManager.js' const { ObjectId } = mongodb const EMPTY_FILE_HASH = 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391' @@ -2379,4 +2387,167 @@ describe('Syncing with web and doc-updater', function () { }) }) }) + + // Regression: UpdateCompressor.compressUpdates optimizes a delete+insert + // at the same position by diffing the two strings and retaining common + // substrings. The TC retain ops are computed by the SyncManager based on + // the original (unoptimized) content diff. When the optimized ops retain + // characters that the original diff deleted, tracked changes on those + // characters survive — but the TC retains don't clear them, producing a + // ghost tracked change. + describe('resync with tracked changes overlapping content diff', function () { + const TIMESTAMP = '2025-01-01T00:00:00.000Z' + + const persisted = { + content: 'lblcdhqcmihkrvzlifscqmwytt\n\ndgakoxboqpzdbbjtom', + trackedChanges: [ + { + range: { pos: 2, length: 3 }, + tracking: { type: 'insert', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 12, length: 4 }, + tracking: { type: 'delete', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 26, length: 3 }, + tracking: { type: 'delete', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 32, length: 5 }, + tracking: { type: 'delete', userId: 'user-2', ts: TIMESTAMP }, + }, + ], + } + + const expected = { + content: 'lblchfjaqdhqcmihkrvzliftt\ndoqpzdbom', + trackedChanges: [ + { + range: { pos: 0, length: 4 }, + tracking: { type: 'insert', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 11, length: 1 }, + tracking: { type: 'delete', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 17, length: 5 }, + tracking: { type: 'delete', userId: 'user-2', ts: TIMESTAMP }, + }, + { + range: { pos: 22, length: 3 }, + tracking: { type: 'insert', userId: 'user-1', ts: TIMESTAMP }, + }, + { + range: { pos: 26, length: 5 }, + tracking: { type: 'delete', userId: 'user-1', ts: TIMESTAMP }, + }, + ], + } + + it('should not leave ghost tracked changes after resync', async function () { + const rangesBlob = JSON.stringify({ + comments: [], + trackedChanges: persisted.trackedChanges, + }) + const contentHash = _getBlobHashFromString(persisted.content) + const rangesHash = _getBlobHashFromString(rangesBlob) + + MockHistoryStore() + .get(`/api/projects/${historyId}/latest/history`) + .reply(200, { + chunk: { + history: { + snapshot: { + files: { + 'main.tex': { + hash: contentHash, + stringLength: persisted.content.length, + rangesHash, + }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }) + + MockHistoryStore() + .get(`/api/projects/${historyId}/blobs/${contentHash}`) + .reply(200, persisted.content) + + MockHistoryStore() + .get(`/api/projects/${historyId}/blobs/${rangesHash}`) + .reply(200, rangesBlob) + + MockHistoryStore() + .put(/\/api\/projects\/[^/]+\/blobs\/[0-9a-f]+/) + .optionally() + .reply(201) + + const allCapturedChanges = [] + MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + allCapturedChanges.push(...body) + return true + }) + .query(true) + .times(5) + .optionally() + .reply(204) + + MockWeb().post(`/project/${this.project_id}/history/resync`).reply(204) + + await ProjectHistoryClient.resyncHistory(this.project_id) + + await ProjectHistoryClient.pushRawUpdate(this.project_id, { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { ts: this.timestamp }, + }) + + await ProjectHistoryClient.pushRawUpdate(this.project_id, { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: expected.content, + historyOTRanges: { + comments: [], + trackedChanges: expected.trackedChanges, + }, + }, + doc: this.doc_id, + meta: { ts: this.timestamp }, + }) + + await ProjectHistoryClient.flushProject(this.project_id) + + const fileData = new StringFileData( + persisted.content, + [], + persisted.trackedChanges + ) + const snapshot = new Snapshot( + new FileMap({ 'main.tex': new File(fileData) }) + ) + for (const rawChange of allCapturedChanges) { + Change.fromRaw(rawChange).applyTo(snapshot) + } + + const file = snapshot.getFile('main.tex') + assert.strictEqual(file.getContent(), expected.content, 'content') + assert.deepStrictEqual( + file.getTrackedChanges().toRaw(), + expected.trackedChanges, + `TC mismatch.\n` + + ` expected: ${JSON.stringify(expected.trackedChanges)}\n` + + ` actual: ${JSON.stringify(file.getTrackedChanges().toRaw())}` + ) + }) + }) }) diff --git a/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js b/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js index 4bb031de64..37a58f7ab0 100644 --- a/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js +++ b/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js @@ -1656,6 +1656,98 @@ describe('UpdateCompressor', function () { ]) }) + it('should not compress when first update is a resync', function () { + expect( + this.UpdateCompressor.compressUpdates([ + { + op: { p: 3, d: 'one two three four five six seven eight' }, + meta: { + ts: this.ts1, + user_id: this.user_id, + doc_length: 100, + resync: true, + }, + v: 42, + }, + { + op: { p: 3, i: 'one 2 three four five six seven eight' }, + meta: { + ts: this.ts2, + user_id: this.user_id, + doc_length: 100, + }, + v: 43, + }, + ]) + ).to.deep.equal([ + { + op: { p: 3, d: 'one two three four five six seven eight' }, + meta: { + ts: this.ts1, + user_id: this.user_id, + doc_length: 100, + resync: true, + }, + v: 42, + }, + { + op: { p: 3, i: 'one 2 three four five six seven eight' }, + meta: { + ts: this.ts2, + user_id: this.user_id, + doc_length: 100, + }, + v: 43, + }, + ]) + }) + + it('should not compress when second update is a resync', function () { + expect( + this.UpdateCompressor.compressUpdates([ + { + op: { p: 3, d: 'one two three four five six seven eight' }, + meta: { + ts: this.ts1, + user_id: this.user_id, + doc_length: 100, + }, + v: 42, + }, + { + op: { p: 3, i: 'one 2 three four five six seven eight' }, + meta: { + ts: this.ts2, + user_id: this.user_id, + doc_length: 100, + resync: true, + }, + v: 43, + }, + ]) + ).to.deep.equal([ + { + op: { p: 3, d: 'one two three four five six seven eight' }, + meta: { + ts: this.ts1, + user_id: this.user_id, + doc_length: 100, + }, + v: 42, + }, + { + op: { p: 3, i: 'one 2 three four five six seven eight' }, + meta: { + ts: this.ts2, + user_id: this.user_id, + doc_length: 100, + resync: true, + }, + v: 43, + }, + ]) + }) + it('special case for delete + insert triggering diff', function () { const meta = { ts: this.ts1, user_id: this.user_id, doc_length: 10 } expect(