From c9e749e4a246f9b390b380f558f99b76adaf6385 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 11:40:20 +0300 Subject: [PATCH] fix(git-sync): don't clobber pages with a live editing session; crash-safe body write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding #5: the git -> page body write (writeBody) did a full-body replace (delete-all + re-insert) on the shared Yjs doc. Applied while a human is editing the page, it discarded their in-flight changes; and TiptapTransformer.toYdoc ran AFTER the fragment was cleared, so a conversion failure could leave the page with an empty body. Fixes: - Active-session guard: CollaborationGateway.getActiveEditorCount(documentName) reports live human (websocket) editor sessions for a doc, excluding server-side direct connections. writeBody now throws ActiveEditSessionError when an editor is connected. The engine's push loop already isolates each importPageMarkdown in try/catch and does not advance the loop-guard on failure, so the write is simply retried on the next poll once the editor disconnects — never a clobber. - Crash-safe conversion: build the replacement Yjs update BEFORE opening the connection / clearing the fragment, so a transform failure can never leave the body empty. Also updates the server-side converter gate spec to the corrected round-trip shape: the block-image hoist no longer leaves a leading empty paragraph (the git-sync converter fix in 7d39c16b, now reaching the built package). A true merge of git content into a live Yjs session is out of scope (it needs a real 3-way text merge with no shared update lineage); deferring the write while a page is being edited is the safe, owner-approved minimum. Co-Authored-By: Claude Opus 4.8 --- .../collaboration/collaboration.gateway.ts | 15 ++++ .../git-sync-converter-gate.spec.ts | 12 ++-- .../gitmost-datasource.service.spec.ts | 69 ++++++++++++++++++- .../services/gitmost-datasource.service.ts | 51 ++++++++++++-- 4 files changed, 133 insertions(+), 14 deletions(-) diff --git a/apps/server/src/collaboration/collaboration.gateway.ts b/apps/server/src/collaboration/collaboration.gateway.ts index b46c13c8..986cc451 100644 --- a/apps/server/src/collaboration/collaboration.gateway.ts +++ b/apps/server/src/collaboration/collaboration.gateway.ts @@ -137,6 +137,21 @@ export class CollaborationGateway { return this.hocuspocus.getDocumentsCount(); } + /** + * Number of LIVE human editor sessions (websocket connections) currently open + * on a document, or 0 if the document is not loaded. Unlike + * `Document.getConnectionsCount()` this deliberately excludes server-side + * direct connections (`directConnectionsCount`, e.g. the git-sync writer + * itself), so callers can tell whether a real person is editing right now. + * + * NOTE: this reflects only THIS instance. In a Redis-clustered deployment an + * editor attached to another node is not counted; for the single-instance + * deployments this guards (git-sync) that is exactly the live set. + */ + getActiveEditorCount(documentName: string): number { + return this.hocuspocus.documents.get(documentName)?.connections.size ?? 0; + } + handleYjsEvent( eventName: TName, documentName: string, diff --git a/apps/server/src/collaboration/git-sync-converter-gate.spec.ts b/apps/server/src/collaboration/git-sync-converter-gate.spec.ts index 603c8f7d..b37d04fa 100644 --- a/apps/server/src/collaboration/git-sync-converter-gate.spec.ts +++ b/apps/server/src/collaboration/git-sync-converter-gate.spec.ts @@ -401,18 +401,19 @@ describe('git-sync converter §13.1 KNOWN DIVERGENCE (markdown image lossiness)' }, }); - it('drops width/height/align (markdown ![](src) cannot carry them) and hoists the block image past a leading empty paragraph', async () => { + it('drops width/height/align (markdown ![](src) cannot carry them); the block-image hoist no longer leaves an empty paragraph', async () => { const { md, canonNormalized } = await runGate(imageDoc); // Export is plain markdown image syntax — no dimensions/align survive. expect(md.trim()).toBe('![](https://example.com/pic.png)'); - // The round-tripped doc is the documented lossy shape: a leading empty - // paragraph (block-image hoist) + an image carrying ONLY src (+ alt=""). + // The round-tripped doc carries ONLY src (+ alt=""). The leading empty + // paragraph that the block-image hoist used to leave behind (a phantom + // blank-gap on every sync) is now stripped on import (git-sync fix), so the + // doc is just the image — no empty-paragraph artifact. expect(canonNormalized).toEqual({ type: 'doc', content: [ - { type: 'paragraph' }, { type: 'image', attrs: { alt: '', src: 'https://example.com/pic.png' }, @@ -420,7 +421,8 @@ describe('git-sync converter §13.1 KNOWN DIVERGENCE (markdown image lossiness)' ], }); - // And it is therefore NOT canonically equal to the original (lock the loss). + // Still NOT canonically equal to the original: width/height/align are an + // intrinsic markdown-transport loss (unrelated to the empty-paragraph fix). expect(docsCanonicallyEqual(imageDoc, canonNormalized)).toBe(false); }); }); diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts index 5e3d103a..572227a6 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts @@ -7,13 +7,35 @@ jest.mock('../../../collaboration/collaboration.util', () => ({ tiptapExtensions: [], getPageId: (name: string) => name.replace(/^page\./, ''), })); +// writeBody now builds the replacement Yjs state eagerly (before clearing the +// live doc), so TiptapTransformer.toYdoc runs in these unit tests. Real Tiptap +// extensions are stubbed to [] above (they drag in the React graph), which can't +// build a schema — so stub the transformer to return a small non-empty Y.Doc. +// The real conversion is exercised by the @docmost/git-sync converter tests and +// the integration tests. +jest.mock('@hocuspocus/transformer', () => { + const Yjs = require('yjs'); + return { + TiptapTransformer: { + toYdoc: jest.fn(() => { + const d = new Yjs.Doc(); + d.getXmlFragment('default').insert(0, [new Yjs.XmlElement('paragraph')]); + return d; + }), + }, + }; +}); // PageService is only ever a mocked dependency here; stub the editor-ext entry // it imports so loading its module does not pull in the React graph either. jest.mock('@docmost/editor-ext', () => ({ markdownToHtml: jest.fn(), })); -import { GitmostDataSourceService } from './gitmost-datasource.service'; +import * as Y from 'yjs'; +import { + GitmostDataSourceService, + ActiveEditSessionError, +} from './gitmost-datasource.service'; // Focused unit/contract test for the native GitSyncClient adapter (plan §3). // No DB, no real collab server: the repos/services/gateway are mocked and we @@ -34,7 +56,10 @@ interface Mocks { movePage: AnyMock; removePage: AnyMock; }; - collabGateway: { openDirectConnection: AnyMock }; + collabGateway: { + openDirectConnection: AnyMock; + getActiveEditorCount: AnyMock; + }; // Minimal Kysely-ish chainable mock for the direct-query paths. db: any; // Captured collab connection (the fake conn the gateway returns). @@ -85,6 +110,8 @@ function build(rows: any[] = []): { conn.context = ctx; return conn; }), + // Default: no live editor sessions, so body writes proceed. + getActiveEditorCount: jest.fn(() => 0), }, db: { selectFrom: jest.fn(() => makeQueryBuilder(rows)), @@ -219,6 +246,44 @@ describe('GitmostDataSourceService', () => { expect(res.updatedAt).toBe('2026-06-20T11:00:00.000Z'); }); + + it('defers (throws ActiveEditSessionError) when a human is editing the page — never clobbers', async () => { + const { service, mocks } = build(); + // A live editor session on this page. + mocks.collabGateway.getActiveEditorCount.mockReturnValue(1); + + await expect( + service.bind(CTX).importPageMarkdown('p1', '# Hello\n\nworld'), + ).rejects.toBeInstanceOf(ActiveEditSessionError); + + // The destructive full-body write must NOT have happened: no connection + // opened, no transact run. The engine's push loop catches this and retries + // on the next poll once the editor disconnects. + expect(mocks.collabGateway.getActiveEditorCount).toHaveBeenCalledWith( + 'page.p1', + ); + expect(mocks.collabGateway.openDirectConnection).not.toHaveBeenCalled(); + expect(mocks.conn.transact).not.toHaveBeenCalled(); + }); + + it('crash-safe: the captured write applies real content (update built before clearing)', async () => { + // The replacement Yjs update is computed BEFORE the connection opens / the + // fragment is cleared, so a transform failure can never leave the body + // emptied. Here we run the captured transact callback against a REAL Y.Doc + // and confirm it ends up with content (the precomputed update is valid and + // applied), i.e. the write produces a non-empty body rather than wiping it. + const { service, mocks } = build(); + mocks.pageRepo.findById.mockResolvedValue({ + id: 'p1', + updatedAt: new Date('2026-06-20T11:00:00.000Z'), + }); + await service.bind(CTX).importPageMarkdown('p1', '# Hi\n\nthere'); + + const realDoc = new Y.Doc(); + expect(() => mocks.conn.capturedFn?.(realDoc)).not.toThrow(); + // The body fragment is non-empty: the markdown was converted and applied. + expect(realDoc.getXmlFragment('default').length).toBeGreaterThan(0); + }); }); describe('createPage', () => { diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts index e317a72d..06fb40b3 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts @@ -42,6 +42,23 @@ const GIT_SYNC_PROVENANCE: AuthProvenanceData = { aiChatId: null, }; +/** + * Thrown when a git -> page body write is skipped because a human is editing the + * page RIGHT NOW (a live collab session). The engine's push loop catches this + * per page, records it as a (non-fatal) failure, and does NOT advance the + * loop-guard for that page — so the write is retried on the next poll once the + * editor disconnects, instead of clobbering their in-flight edits with a + * full-body replace (plan §15.6 / review #5). + */ +export class ActiveEditSessionError extends Error { + constructor(pageId: string) { + super( + `git-sync: page ${pageId} has an active edit session; deferring body write`, + ); + this.name = 'ActiveEditSessionError'; + } +} + /** * Native, in-process implementation of the engine's `GitSyncClient` seam * (plan §3). Reads go through repositories (PageRepo/SpaceRepo); body writes go @@ -379,7 +396,32 @@ export class GitmostDataSourceService { prosemirrorJson: unknown, userId: string, ): Promise { - const conn = await this.collabGateway.openDirectConnection(`page.${pageId}`, { + const documentName = `page.${pageId}`; + + // Do NOT clobber a page someone is editing right now. The write below is a + // full-body replace (delete-all + re-insert); applied over a live editing + // session it would discard the user's in-flight changes. If a human editor + // is connected, defer: throw so the engine retries on the next poll once + // they disconnect (review #5 — "не писать в страницу с активной сессией"). + if (this.collabGateway.getActiveEditorCount(documentName) > 0) { + this.logger.debug( + `Skipping git-sync body write for ${documentName}: active edit session`, + ); + throw new ActiveEditSessionError(pageId); + } + + // Build the replacement Yjs state BEFORE touching the live doc. If the + // transform throws (a malformed/unsupported doc), we must NOT have already + // cleared the fragment — otherwise a conversion failure would leave the page + // with an empty body (review #5 — crash-safe conversion). + const next = TiptapTransformer.toYdoc( + prosemirrorJson, + 'default', + tiptapExtensions, + ); + const update = Y.encodeStateAsUpdate(next); + + const conn = await this.collabGateway.openDirectConnection(documentName, { actor: 'git-sync', // PersistenceExtension reads `context.user.id` for lastUpdatedById, so the // service user is required on the context (unlike the bare `{ actor }` @@ -390,12 +432,7 @@ export class GitmostDataSourceService { await conn.transact((doc) => { const fragment = doc.getXmlFragment('default'); if (fragment.length > 0) fragment.delete(0, fragment.length); - const next = TiptapTransformer.toYdoc( - prosemirrorJson, - 'default', - tiptapExtensions, - ); - Y.applyUpdate(doc, Y.encodeStateAsUpdate(next)); + Y.applyUpdate(doc, update); }); } finally { await conn.disconnect();