[project-history] add support for resync of history-ot ranges (#26475)

* [project-history] add support for resync of history-ot ranges * [project-history] avoid compressing sharejs and history-ot upgrades * [document-updater] improve error message of some assertions ... by migrating the assertions like this: ```diff -stub.calledWith().should.equal(true) +stub.should.have.been.calledWith() ``` ```diff -stub.called.should.equal(false) +stub.should.not.have.been.called ``` * [document-updater] move content field in resyncDocContent * [document-updater] add support for resync of history-ot ranges GitOrigin-RevId: e6104686a26934a5f25a8f095cbe00c163fbbaa7
2026-05-24 17:51:51 +02:00 · 2025-06-18 11:24:30 +02:00
parent f5acd16b47
commit dc9e7bd12d
9 changed files with 1579 additions and 74 deletions
--- a/services/document-updater/app/js/DocumentManager.js
+++ b/services/document-updater/app/js/DocumentManager.js
@@ -535,11 +535,6 @@ const DocumentManager = {
    if (opts.historyRangesMigration) {
      historyRangesSupport = opts.historyRangesMigration === 'forwards'
    }
-    if (!Array.isArray(lines)) {
-      const file = StringFileData.fromRaw(lines)
-      // TODO(24596): tc support for history-ot
-      lines = file.getLines()
-    }

    await ProjectHistoryRedisManager.promises.queueResyncDocContent(
      projectId,
--- a/services/document-updater/app/js/Limits.js
+++ b/services/document-updater/app/js/Limits.js
@@ -28,4 +28,19 @@ module.exports = {
    // since we didn't hit the limit in the loop, the document is within the allowed length
    return false
  },
+
+  /**
+   * @param {StringFileRawData} raw
+   * @param {number} maxDocLength
+   */
+  stringFileDataContentIsTooLarge(raw, maxDocLength) {
+    let n = raw.content.length
+    if (n <= maxDocLength) return false // definitely under the limit, no need to calculate the total size
+    for (const tc of raw.trackedChanges ?? []) {
+      if (tc.tracking.type !== 'delete') continue
+      n -= tc.range.length
+      if (n <= maxDocLength) return false // under the limit now, no need to calculate the exact size
+    }
+    return true
+  },
 }
--- a/services/document-updater/app/js/ProjectHistoryRedisManager.js
+++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js
@@ -8,13 +8,14 @@ const rclient = require('@overleaf/redis-wrapper').createClient(
 )
 const logger = require('@overleaf/logger')
 const metrics = require('./Metrics')
-const { docIsTooLarge } = require('./Limits')
+const { docIsTooLarge, stringFileDataContentIsTooLarge } = require('./Limits')
 const { addTrackedDeletesToContent, extractOriginOrSource } = require('./Utils')
 const HistoryConversions = require('./HistoryConversions')
 const OError = require('@overleaf/o-error')

 /**
 * @import { Ranges } from './types'
+ * @import { StringFileRawData } from 'overleaf-editor-core/lib/types'
 */

 const ProjectHistoryRedisManager = {
@@ -180,7 +181,7 @@ const ProjectHistoryRedisManager = {
   * @param {string} projectId
   * @param {string} projectHistoryId
   * @param {string} docId
-   * @param {string[]} lines
+   * @param {string[] | StringFileRawData} lines
   * @param {Ranges} ranges
   * @param {string[]} resolvedCommentIds
   * @param {number} version
@@ -204,13 +205,8 @@ const ProjectHistoryRedisManager = {
      'queue doc content resync'
    )

-    let content = lines.join('\n')
-    if (historyRangesSupport) {
-      content = addTrackedDeletesToContent(content, ranges.changes ?? [])
-    }
-
    const projectUpdate = {
-      resyncDocContent: { content, version },
+      resyncDocContent: { version },
      projectHistoryId,
      path: pathname,
      doc: docId,
@@ -219,17 +215,38 @@ const ProjectHistoryRedisManager = {
      },
    }

-    if (historyRangesSupport) {
-      projectUpdate.resyncDocContent.ranges =
-        HistoryConversions.toHistoryRanges(ranges)
-      projectUpdate.resyncDocContent.resolvedCommentIds = resolvedCommentIds
+    let content = ''
+    if (Array.isArray(lines)) {
+      content = lines.join('\n')
+      if (historyRangesSupport) {
+        content = addTrackedDeletesToContent(content, ranges.changes ?? [])
+        projectUpdate.resyncDocContent.ranges =
+          HistoryConversions.toHistoryRanges(ranges)
+        projectUpdate.resyncDocContent.resolvedCommentIds = resolvedCommentIds
+      }
+    } else {
+      content = lines.content
+      projectUpdate.resyncDocContent.historyOTRanges = {
+        comments: lines.comments,
+        trackedChanges: lines.trackedChanges,
+      }
    }
+    projectUpdate.resyncDocContent.content = content

    const jsonUpdate = JSON.stringify(projectUpdate)
    // Do an optimised size check on the docLines using the serialised
    // project update length as an upper bound
    const sizeBound = jsonUpdate.length
-    if (docIsTooLarge(sizeBound, lines, Settings.max_doc_length)) {
+    if (Array.isArray(lines)) {
+      if (docIsTooLarge(sizeBound, lines, Settings.max_doc_length)) {
+        throw new OError(
+          'blocking resync doc content insert into project history queue: doc is too large',
+          { projectId, docId, docSize: sizeBound }
+        )
+      }
+    } else if (
+      stringFileDataContentIsTooLarge(lines, Settings.max_doc_length)
+    ) {
      throw new OError(
        'blocking resync doc content insert into project history queue: doc is too large',
        { projectId, docId, docSize: sizeBound }
--- a/services/document-updater/test/unit/js/Limits/LimitsTests.js
+++ b/services/document-updater/test/unit/js/Limits/LimitsTests.js
@@ -81,4 +81,88 @@ describe('Limits', function () {
      })
    })
  })
+
+  describe('stringFileDataContentIsTooLarge', function () {
+    it('should handle small docs', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge({ content: '' }, 123)
+      ).to.equal(false)
+    })
+    it('should handle docs at the limit', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge(
+          { content: 'x'.repeat(123) },
+          123
+        )
+      ).to.equal(false)
+    })
+    it('should handle docs above the limit', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge(
+          { content: 'x'.repeat(123 + 1) },
+          123
+        )
+      ).to.equal(true)
+    })
+    it('should handle docs above the limit and below with tracked-deletes removed', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge(
+          {
+            content: 'x'.repeat(123 + 1),
+            trackedChanges: [
+              {
+                range: { pos: 1, length: 1 },
+                tracking: {
+                  type: 'delete',
+                  ts: '2025-06-16T14:31:44.910Z',
+                  userId: 'user-id',
+                },
+              },
+            ],
+          },
+          123
+        )
+      ).to.equal(false)
+    })
+    it('should handle docs above the limit and above with tracked-deletes removed', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge(
+          {
+            content: 'x'.repeat(123 + 2),
+            trackedChanges: [
+              {
+                range: { pos: 1, length: 1 },
+                tracking: {
+                  type: 'delete',
+                  ts: '2025-06-16T14:31:44.910Z',
+                  userId: 'user-id',
+                },
+              },
+            ],
+          },
+          123
+        )
+      ).to.equal(true)
+    })
+    it('should handle docs above the limit and with tracked-inserts', function () {
+      expect(
+        this.Limits.stringFileDataContentIsTooLarge(
+          {
+            content: 'x'.repeat(123 + 1),
+            trackedChanges: [
+              {
+                range: { pos: 1, length: 1 },
+                tracking: {
+                  type: 'insert',
+                  ts: '2025-06-16T14:31:44.910Z',
+                  userId: 'user-id',
+                },
+              },
+            ],
+          },
+          123
+        )
+      ).to.equal(true)
+    })
+  })
 })
--- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js
+++ b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js
@@ -15,6 +15,7 @@ describe('ProjectHistoryRedisManager', function () {

    this.Limits = {
      docIsTooLarge: sinon.stub().returns(false),
+      stringFileDataContentIsTooLarge: sinon.stub().returns(false),
    }

    this.ProjectHistoryRedisManager = SandboxedModule.require(modulePath, {
@@ -61,22 +62,18 @@ describe('ProjectHistoryRedisManager', function () {
    })

    it('should queue an update', function () {
-      this.multi.rpush
-        .calledWithExactly(
-          `ProjectHistory:Ops:${this.project_id}`,
-          this.ops[0],
-          this.ops[1]
-        )
-        .should.equal(true)
+      this.multi.rpush.should.have.been.calledWithExactly(
+        `ProjectHistory:Ops:${this.project_id}`,
+        this.ops[0],
+        this.ops[1]
+      )
    })

    it('should set the queue timestamp if not present', function () {
-      this.multi.setnx
-        .calledWithExactly(
-          `ProjectHistory:FirstOpTimestamp:${this.project_id}`,
-          Date.now()
-        )
-        .should.equal(true)
+      this.multi.setnx.should.have.been.calledWithExactly(
+        `ProjectHistory:FirstOpTimestamp:${this.project_id}`,
+        Date.now()
+      )
    })
  })

@@ -118,9 +115,10 @@ describe('ProjectHistoryRedisManager', function () {
        file: this.file_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })
  })

@@ -166,9 +164,10 @@ describe('ProjectHistoryRedisManager', function () {
        doc: this.doc_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })

    it('should queue an update with file metadata', async function () {
@@ -350,9 +349,10 @@ describe('ProjectHistoryRedisManager', function () {
        doc: this.doc_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })

    it('should not forward ranges if history ranges support is undefined', async function () {
@@ -402,9 +402,10 @@ describe('ProjectHistoryRedisManager', function () {
        doc: this.doc_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })

    it('should pass "false" as the createdBlob field if not provided', async function () {
@@ -432,9 +433,10 @@ describe('ProjectHistoryRedisManager', function () {
        doc: this.doc_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })

    it('should pass through the value of the createdBlob field', async function () {
@@ -463,9 +465,10 @@ describe('ProjectHistoryRedisManager', function () {
        doc: this.doc_id,
      }

-      this.ProjectHistoryRedisManager.promises.queueOps
-        .calledWithExactly(this.project_id, JSON.stringify(update))
-        .should.equal(true)
+      this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+        this.project_id,
+        JSON.stringify(update)
+      )
    })
  })

@@ -493,8 +496,8 @@ describe('ProjectHistoryRedisManager', function () {
      beforeEach(async function () {
        this.update = {
          resyncDocContent: {
-            content: 'one\ntwo',
            version: this.version,
+            content: 'one\ntwo',
          },
          projectHistoryId: this.projectHistoryId,
          path: this.pathname,
@@ -516,19 +519,18 @@ describe('ProjectHistoryRedisManager', function () {
      })

      it('should check if the doc is too large', function () {
-        this.Limits.docIsTooLarge
-          .calledWith(
-            JSON.stringify(this.update).length,
-            this.lines,
-            this.settings.max_doc_length
-          )
-          .should.equal(true)
+        this.Limits.docIsTooLarge.should.have.been.calledWith(
+          JSON.stringify(this.update).length,
+          this.lines,
+          this.settings.max_doc_length
+        )
      })

      it('should queue an update', function () {
-        this.ProjectHistoryRedisManager.promises.queueOps
-          .calledWithExactly(this.project_id, JSON.stringify(this.update))
-          .should.equal(true)
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+          this.project_id,
+          JSON.stringify(this.update)
+        )
      })
    })

@@ -551,9 +553,8 @@ describe('ProjectHistoryRedisManager', function () {
      })

      it('should not queue an update if the doc is too large', function () {
-        this.ProjectHistoryRedisManager.promises.queueOps.called.should.equal(
-          false
-        )
+        this.ProjectHistoryRedisManager.promises.queueOps.should.not.have.been
+          .called
      })
    })

@@ -561,10 +562,10 @@ describe('ProjectHistoryRedisManager', function () {
      beforeEach(async function () {
        this.update = {
          resyncDocContent: {
-            content: 'onedeleted\ntwo',
            version: this.version,
            ranges: this.ranges,
            resolvedCommentIds: this.resolvedCommentIds,
+            content: 'onedeleted\ntwo',
          },
          projectHistoryId: this.projectHistoryId,
          path: this.pathname,
@@ -601,9 +602,76 @@ describe('ProjectHistoryRedisManager', function () {
      })

      it('should queue an update', function () {
-        this.ProjectHistoryRedisManager.promises.queueOps
-          .calledWithExactly(this.project_id, JSON.stringify(this.update))
-          .should.equal(true)
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+          this.project_id,
+          JSON.stringify(this.update)
+        )
+      })
+    })
+
+    describe('history-ot', function () {
+      beforeEach(async function () {
+        this.lines = {
+          content: 'onedeleted\ntwo',
+          comments: [{ id: 'id1', ranges: [{ pos: 0, length: 3 }] }],
+          trackedChanges: [
+            {
+              range: { pos: 3, length: 7 },
+              tracking: {
+                type: 'delete',
+                userId: 'user-id',
+                ts: '2025-06-16T14:31:44.910Z',
+              },
+            },
+          ],
+        }
+        this.update = {
+          resyncDocContent: {
+            version: this.version,
+            historyOTRanges: {
+              comments: this.lines.comments,
+              trackedChanges: this.lines.trackedChanges,
+            },
+            content: this.lines.content,
+          },
+          projectHistoryId: this.projectHistoryId,
+          path: this.pathname,
+          doc: this.doc_id,
+          meta: { ts: new Date() },
+        }
+
+        await this.ProjectHistoryRedisManager.promises.queueResyncDocContent(
+          this.project_id,
+          this.projectHistoryId,
+          this.doc_id,
+          this.lines,
+          this.ranges,
+          this.resolvedCommentIds,
+          this.version,
+          this.pathname,
+          true
+        )
+      })
+
+      it('should include tracked deletes in the update', function () {
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+          this.project_id,
+          JSON.stringify(this.update)
+        )
+      })
+
+      it('should check the doc length without tracked deletes', function () {
+        this.Limits.stringFileDataContentIsTooLarge.should.have.been.calledWith(
+          this.lines,
+          this.settings.max_doc_length
+        )
+      })
+
+      it('should queue an update', function () {
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly(
+          this.project_id,
+          JSON.stringify(this.update)
+        )
      })
    })
  })
--- a/services/project-history/app/js/SyncManager.js
+++ b/services/project-history/app/js/SyncManager.js
@@ -23,6 +23,7 @@ import { isInsert, isDelete } from './Utils.js'

 /**
 * @import { Comment as HistoryComment, TrackedChange as HistoryTrackedChange } from 'overleaf-editor-core'
+ * @import { CommentRawData, TrackedChangeRawData } from 'overleaf-editor-core/lib/types'
 * @import { Comment, Entity, ResyncDocContentUpdate, RetainOp, TrackedChange } from './types'
 * @import { TrackedChangeTransition, TrackingDirective, TrackingType, Update } from './types'
 * @import { ProjectStructureUpdate } from './types'
@@ -764,11 +765,19 @@ class SyncUpdateExpander {
    }

    const persistedComments = file.getComments().toArray()
-    await this.queueUpdatesForOutOfSyncComments(
-      update,
-      pathname,
-      persistedComments
-    )
+    if (update.resyncDocContent.historyOTRanges) {
+      this.queueUpdatesForOutOfSyncCommentsHistoryOT(
+        update,
+        pathname,
+        file.getComments().toRaw()
+      )
+    } else {
+      await this.queueUpdatesForOutOfSyncComments(
+        update,
+        pathname,
+        persistedComments
+      )
+    }

    const persistedChanges = file.getTrackedChanges().asSorted()
    await this.queueUpdatesForOutOfSyncTrackedChanges(
@@ -825,6 +834,91 @@ class SyncUpdateExpander {
    return expandedUpdate
  }

+  /**
+   * Queue updates for out of sync comments
+   *
+   * @param {ResyncDocContentUpdate} update
+   * @param {string} pathname
+   * @param {CommentRawData[]} persistedComments
+   */
+  queueUpdatesForOutOfSyncCommentsHistoryOT(
+    update,
+    pathname,
+    persistedComments
+  ) {
+    const expectedComments =
+      update.resyncDocContent.historyOTRanges?.comments ?? []
+    const expectedCommentsById = new Map(
+      expectedComments.map(comment => [comment.id, comment])
+    )
+    const persistedCommentsById = new Map(
+      persistedComments.map(comment => [comment.id, comment])
+    )
+
+    // Delete any persisted comment that is not in the expected comment list.
+    for (const persistedComment of persistedComments) {
+      if (!expectedCommentsById.has(persistedComment.id)) {
+        this.expandedUpdates.push({
+          doc: update.doc,
+          op: [{ deleteComment: persistedComment.id }],
+          meta: {
+            pathname,
+            resync: true,
+            origin: this.origin,
+            ts: update.meta.ts,
+          },
+        })
+      }
+    }
+
+    for (const expectedComment of expectedComments) {
+      const persistedComment = persistedCommentsById.get(expectedComment.id)
+      if (
+        persistedComment &&
+        commentRangesAreInSyncHistoryOT(persistedComment, expectedComment)
+      ) {
+        if (expectedComment.resolved === persistedComment.resolved) {
+          // Both comments are identical; do nothing
+        } else {
+          // Only the resolved state differs
+          this.expandedUpdates.push({
+            doc: update.doc,
+            op: [
+              {
+                commentId: expectedComment.id,
+                resolved: expectedComment.resolved,
+              },
+            ],
+            meta: {
+              pathname,
+              resync: true,
+              origin: this.origin,
+              ts: update.meta.ts,
+            },
+          })
+        }
+      } else {
+        // New comment or ranges differ
+        this.expandedUpdates.push({
+          doc: update.doc,
+          op: [
+            {
+              commentId: expectedComment.id,
+              ranges: expectedComment.ranges,
+              resolved: expectedComment.resolved,
+            },
+          ],
+          meta: {
+            pathname,
+            resync: true,
+            origin: this.origin,
+            ts: update.meta.ts,
+          },
+        })
+      }
+    }
+  }
+
  /**
   * Queue updates for out of sync comments
   *
@@ -951,6 +1045,7 @@ class SyncUpdateExpander {
    for (const transition of getTrackedChangesTransitions(
      persistedChanges,
      expectedChanges,
+      update.resyncDocContent.historyOTRanges?.trackedChanges || [],
      expectedContent.length
    )) {
      if (transition.pos > cursor) {
@@ -1018,6 +1113,25 @@ class SyncUpdateExpander {
  }
 }

+/**
+ * Compares the ranges in the persisted and expected comments
+ *
+ * @param {CommentRawData} persistedComment
+ * @param {CommentRawData} expectedComment
+ */
+function commentRangesAreInSyncHistoryOT(persistedComment, expectedComment) {
+  if (persistedComment.ranges.length !== expectedComment.ranges.length) {
+    return false
+  }
+  for (let i = 0; i < persistedComment.ranges.length; i++) {
+    const persistedRange = persistedComment.ranges[i]
+    const expectedRange = expectedComment.ranges[i]
+    if (persistedRange.pos !== expectedRange.pos) return false
+    if (persistedRange.length !== expectedRange.length) return false
+  }
+  return true
+}
+
 /**
 * Compares the ranges in the persisted and expected comments
 *
@@ -1049,11 +1163,13 @@ function commentRangesAreInSync(persistedComment, expectedComment) {
 *
 * @param {readonly HistoryTrackedChange[]} persistedChanges
 * @param {TrackedChange[]} expectedChanges
+ * @param {TrackedChangeRawData[]} persistedChangesHistoryOT
 * @param {number} docLength
 */
 function getTrackedChangesTransitions(
  persistedChanges,
  expectedChanges,
+  persistedChangesHistoryOT,
  docLength
 ) {
  /** @type {TrackedChangeTransition[]} */
@@ -1076,6 +1192,19 @@ function getTrackedChangesTransitions(
    })
  }

+  for (const change of persistedChangesHistoryOT) {
+    transitions.push({
+      stage: 'expected',
+      pos: change.range.pos,
+      tracking: change.tracking,
+    })
+    transitions.push({
+      stage: 'expected',
+      pos: change.range.pos + change.range.length,
+      tracking: { type: 'none' },
+    })
+  }
+
  for (const change of expectedChanges) {
    const op = change.op
    const pos = op.hpos ?? op.p
--- a/services/project-history/app/js/UpdateCompressor.js
+++ b/services/project-history/app/js/UpdateCompressor.js
@@ -169,7 +169,9 @@ export function concatUpdatesWithSameVersion(updates) {
        lastUpdate.op != null &&
        lastUpdate.v === update.v &&
        lastUpdate.doc === update.doc &&
-        lastUpdate.pathname === update.pathname
+        lastUpdate.pathname === update.pathname &&
+        EditOperationBuilder.isValid(update.op[0]) ===
+          EditOperationBuilder.isValid(lastUpdate.op[0])
      ) {
        lastUpdate.op = lastUpdate.op.concat(update.op)
        if (update.meta.doc_hash == null) {
--- a/services/project-history/app/js/types.ts
+++ b/services/project-history/app/js/types.ts
@@ -3,6 +3,8 @@ import {
  LinkedFileData,
  RawEditOperation,
  RawOrigin,
+  CommentRawData,
+  TrackedChangeRawData,
 } from 'overleaf-editor-core/lib/types'

 export type Update =
@@ -118,6 +120,10 @@ export type ResyncDocContentUpdate = {
    content: string
    version: number
    ranges?: Ranges
+    historyOTRanges?: {
+      comments: CommentRawData[]
+      trackedChanges: TrackedChangeRawData[]
+    }
    resolvedCommentIds?: string[]
  }
  projectHistoryId: string
--- a/services/project-history/test/acceptance/js/SyncTests.js
+++ b/services/project-history/test/acceptance/js/SyncTests.js