From 255413c0e67f8286388147487fe3eacb45e626e2 Mon Sep 17 00:00:00 2001 From: William Stein Date: Fri, 2 Oct 2026 06:57:39 +0000 Subject: [PATCH 1/2] Mark patches committed without exact values; read only unmarked merges as patchflow 0.8 wrote them Found by the Markdown meeting test on 0.11.0 (10 users, 15 min): 40 inconsistencies and words lost by merging. Sessions that reloaded during heavy concurrency committed without exact values (no hash) for a while; their merge commits looked like patchflow 0.8 merges and were read by replaying the history in time order, whose result depends on how much of the history a client has loaded. Clients disagreed about those values, and a merge commit recording its author's (wrong) merged value made it everyone's, deleting 8 words in one merge. Session now marks a patch committed without the exact value of its parents `inexact` (it has no hash either). Such a merge commit is read with merge3, like any merge commit of this version, which does not depend on the loaded history. Only unmarked merge commits without a hash or recorded value (written by patchflow 0.8) are replayed in time order. Test: a session that cannot compute the exact merge commits an inexact merge, read with merge3 (the time-order replay gives a different value; the test fails when the marker is ignored). Tests that simulate patchflow 0.8 writers strip the marker, as 0.8 never wrote it. --- src/merge-commit.test.ts | 58 +++++++++++++++++++++++++++++++++++++--- src/patch-graph.ts | 16 ++++++----- src/session.ts | 2 ++ src/types.ts | 6 +++++ 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/src/merge-commit.test.ts b/src/merge-commit.test.ts index 7e5fd7d..e38a706 100644 --- a/src/merge-commit.test.ts +++ b/src/merge-commit.test.ts @@ -4,6 +4,7 @@ import { MemoryPatchStore } from "./adapters/memory-patch-store"; import { mergeStrings3 } from "./merge3"; import { StringCodec, StringDocument } from "./string-document"; import { legacyPatchId } from "./patch-id"; +import { hashString } from "./value-hash"; import type { DocCodec, Inconsistency, Patch, PatchEnvelope } from "./types"; const doc = (s: string) => new StringDocument(s); @@ -65,6 +66,9 @@ async function writers(c: DocCodec, n: number) { const strip = ({ mergeParent: _p, mergePatch: _m, ...rest }: PatchEnvelope): Patch => rest; +// A patch as patchflow 0.8 wrote it: no inexact marker. +const as08 = ({ inexact: _inexact, ...rest }: PatchEnvelope): PatchEnvelope => rest; + describe("merge commits record their merged value", () => { async function history() { const [a, b] = await writers(codec, 2); @@ -179,7 +183,7 @@ describe("a history written before merge commits recorded their merged value", ( // Concurrent edits of the same words, which merge differently. const k = Math.floor(rng() * words.length); words[k] = `${words[k]}${step}`; - all.push(s.commit(doc(words.join(" ")))); + all.push(as08(s.commit(doc(words.join(" "))))); } return all; } @@ -268,7 +272,7 @@ describe("merge commits written before hashes", () => { a.applyRemote(r); const m = a.commit(doc(`${a.getDocument()}M${i}\n`)); b.applyRemoteBatch([l, m]); - all.push(l, r, m); + all.push(...[l, r, m].map(as08)); values.set(m.time, `${a.getDocument()}`); } expect(all.filter((p) => (p.parents?.length ?? 0) > 1 && p.hash == null)).toHaveLength(rounds); @@ -349,7 +353,7 @@ describe("replay checkpoints of a large document", () => { a.applyRemote(r); const m = a.commit(doc(`${a.getDocument()}M${i}\n`)); b.applyRemoteBatch([l, m]); - all.push(l, r, m); + all.push(...[l, r, m].map(as08)); values.set(m.time, `${a.getDocument()}`); } let applied = 0; @@ -380,3 +384,51 @@ describe("replay checkpoints of a large document", () => { expect(applied).toBeLessThan(4 * all.length); }); }); + +describe("a merge commit made without the exact value of its parents", () => { + const id = legacyPatchId; + const patch = (t: number, parents: number[], from: string, to: string): Patch => ({ + time: id(t), + parents: parents.map(id), + patch: doc(from).makePatch(doc(to)), + hash: hashString(to), + }); + // 2 deletes a line that 3 edits concurrently; 4 merges them (the edit + // wins) and is snapshotted; 5 edits a line that 6 deletes concurrently. + const ps = [ + patch(1, [], "", "a\nb\nc\n"), + patch(2, [1], "a\nb\nc\n", "a\nc\n"), + patch(3, [1], "a\nb\nc\n", "a\nB\nc\n"), + patch(4, [2, 3], "a\nB\nc\n", "a\nB\nc\nd\n"), + patch(5, [4], "a\nB\nc\nd\n", "a\nB\nC!\nd\n"), + patch(6, [3], "a\nB\nc\n", "a\nB\n"), + ]; + const snapshot: Patch = { ...ps[3], isSnapshot: true, snapshot: "a\nB\nc\nd\n" }; + + it("is marked inexact, and read with merge3", async () => { + // A client with the snapshot, 5 and 6, but not 3 (below the snapshot): + // it cannot compute the exact merge of 5 and 6. + const s = new Session({ + codec, + patchStore: new MemoryPatchStore([snapshot, ps[4], ps[5]]), + clientId: "late", + }); + await s.init(); + expect(s.getHeads()).toEqual([id(5), id(6)].sort()); + const merge = s.commit(doc(`${s.getDocument()}g\n`)); + s.close(); + expect(merge.parents).toHaveLength(2); + expect(merge.hash).toBeUndefined(); + expect(merge.mergeParent).toBeUndefined(); + expect(merge.inexact).toBe(true); + // Read with merge3, like any merge commit of this version, not by + // replaying the history in time order (which depends on the history a + // client has loaded, and here differs). + const full = graph(codec, [...ps, snapshot, merge]); + const value = full.version(merge.time).toString(); + const merged = full.exactValueOf(merge.parents!)!; + expect(value).toBe(codec.applyPatch(merged, merge.patch).toString()); + const legacy = graph(codec, [...ps, snapshot, { ...merge, inexact: undefined }]); + expect(legacy.version(merge.time).toString()).not.toBe(value); + }); +}); diff --git a/src/patch-graph.ts b/src/patch-graph.ts index 316afbf..9b21a9f 100644 --- a/src/patch-graph.ts +++ b/src/patch-graph.ts @@ -60,17 +60,21 @@ function isRoot(patch: Patch): boolean { } // A merge commit (several parents) that does not record its merged value -// (see Patch.mergeParent) and has no hash, so was written by a version that -// applied every patch in time order (patchflow 0.8 and earlier; 0.9 was never -// used in production), or with the value of that same fallback (see -// Session.commit). Its value is computed the way its author computed the value -// it is a diff from (see legacyValueOfSet), not with merge3, so a history -// written before exact merges keeps its values. +// (see Patch.mergeParent), has no hash and is not marked inexact, so was +// written by a version that applied every patch in time order (patchflow 0.8 +// and earlier; 0.9 was never used in production). Its value is computed the +// way its author computed the value it is a diff from (see legacyValueOfSet), +// not with merge3, so a history written before exact merges keeps its values. +// A merge commit written by this version without the exact value of its +// parents is marked inexact (Patch.inexact) and read with merge3, which, unlike +// replaying the history in time order, does not depend on how much of the +// history a client has loaded. function isLegacyMerge(patch: Patch): boolean { return ( (patch.parents?.length ?? 0) > 1 && patch.mergeParent == null && patch.hash == null && + !patch.inexact && patch.patch != null ); } diff --git a/src/session.ts b/src/session.ts index a932993..08ecd72 100644 --- a/src/session.ts +++ b/src/session.ts @@ -367,6 +367,7 @@ export class Session extends EventEmitter { source: opts.source, meta: opts.meta, hash: parentsValue == null ? undefined : this.graph.hashOf(nextDoc), + ...(parentsValue == null ? { inexact: true } : {}), ...this.recordMerge(parents, parentsValue), }; this.graph.add([envelope]); @@ -743,6 +744,7 @@ export class Session extends EventEmitter { version: nextVersion, file: true, hash: parentsValue == null ? undefined : this.graph.hashOf(newDoc), + ...(parentsValue == null ? { inexact: true } : {}), ...this.recordMerge(parents, parentsValue), }; this.graph.add([envelope]); diff --git a/src/types.ts b/src/types.ts index 5061d0a..9cc6a3a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -54,6 +54,12 @@ export interface Patch { // patch-graph.ts. mergeParent?: PatchId; mergePatch?: unknown; + // Set by this version on a patch committed without the exact value of its + // parents (e.g. before the history needed for it was loaded): `patch` is a + // diff from an approximation of that value, and the patch has no hash. It + // tells such a patch apart from one written by a version before exact + // merges (see isLegacyMerge in patch-graph.ts). + inexact?: boolean; } // A value that does not match the hash its author recorded (Patch.hash). From 5ed0196f593afa5105d4a760dbcef7c73d454ecc Mon Sep 17 00:00:00 2001 From: William Stein Date: Fri, 2 Oct 2026 16:08:44 +0000 Subject: [PATCH 2/2] Mark inexact only in sessions that compute exact values Review of #19 (P2): a patch was marked inexact whenever its parents had no exact value, which is also always the case for a session that never computes exact values (a codec without merge3, or the apply-all strategy). Its merge commits are genuine time-order history, but the marker made exact readers read them with merge3, changing their values. Now only a session that computes exact values (PatchGraph.computesExactValues) and lacks them for a commit marks it. Test: the reviewer's case, a current session without merge3 writing a concurrent history, read unchanged by exact readers (merge3 and a different merge): every value is the author's, no patch is marked. The tests that simulated patchflow 0.8 writers no longer strip the marker (fails with the previous rule). --- src/merge-commit.test.ts | 28 ++++++++++++++++++++++------ src/patch-graph.ts | 7 +++++++ src/session.ts | 13 +++++++++++-- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/merge-commit.test.ts b/src/merge-commit.test.ts index e38a706..38c1bd3 100644 --- a/src/merge-commit.test.ts +++ b/src/merge-commit.test.ts @@ -66,9 +66,6 @@ async function writers(c: DocCodec, n: number) { const strip = ({ mergeParent: _p, mergePatch: _m, ...rest }: PatchEnvelope): Patch => rest; -// A patch as patchflow 0.8 wrote it: no inexact marker. -const as08 = ({ inexact: _inexact, ...rest }: PatchEnvelope): PatchEnvelope => rest; - describe("merge commits record their merged value", () => { async function history() { const [a, b] = await writers(codec, 2); @@ -183,11 +180,30 @@ describe("a history written before merge commits recorded their merged value", ( // Concurrent edits of the same words, which merge differently. const k = Math.floor(rng() * words.length); words[k] = `${words[k]}${step}`; - all.push(as08(s.commit(doc(words.join(" "))))); + all.push(s.commit(doc(words.join(" ")))); } return all; } + it("is what a current session without merge3 writes: no inexact marker", async () => { + // The reviewer's case: a current apply-all writer, read by exact readers + // without changing its envelopes. + const [a, b] = await writers(applyAllCodec, 2); + const root = a.commit(doc("hello\n")); + b.applyRemote(root); + const left = a.commit(doc("helloA\n")); + const right = b.commit(doc("helloB\n")); + a.applyRemote(right); + const authored = `${a.getDocument()}tail\n`; + const last = a.commit(doc(authored)); + a.close(); + b.close(); + for (const p of [root, left, right, last]) expect(p.inexact).toBeUndefined(); + for (const c of [codec, otherCodec]) { + expect(graph(c, [root, left, right, last]).version(last.time).toString()).toBe(authored); + } + }); + it("keeps the values its authors saw, whatever the merge algorithm", async () => { let merges = 0; for (let seed = 1; seed <= 20; seed++) { @@ -272,7 +288,7 @@ describe("merge commits written before hashes", () => { a.applyRemote(r); const m = a.commit(doc(`${a.getDocument()}M${i}\n`)); b.applyRemoteBatch([l, m]); - all.push(...[l, r, m].map(as08)); + all.push(l, r, m); values.set(m.time, `${a.getDocument()}`); } expect(all.filter((p) => (p.parents?.length ?? 0) > 1 && p.hash == null)).toHaveLength(rounds); @@ -353,7 +369,7 @@ describe("replay checkpoints of a large document", () => { a.applyRemote(r); const m = a.commit(doc(`${a.getDocument()}M${i}\n`)); b.applyRemoteBatch([l, m]); - all.push(...[l, r, m].map(as08)); + all.push(l, r, m); values.set(m.time, `${a.getDocument()}`); } let applied = 0; diff --git a/src/patch-graph.ts b/src/patch-graph.ts index 9b21a9f..8aabe32 100644 --- a/src/patch-graph.ts +++ b/src/patch-graph.ts @@ -780,6 +780,13 @@ export class PatchGraph { return hashString(this.codec.toString(doc)); } + // Whether this graph computes exact values (the codec provides merge3 and + // the strategy is not apply-all). Otherwise every value applies all patches + // in time order, as patchflow 0.8 did. + computesExactValues(): boolean { + return this.codec.merge3 != null && this.mergeStrategy !== "apply-all"; + } + // The exact merged value of a set of patches (e.g. the parents of a new // patch), or undefined if it cannot be computed from the loaded history or // the graph does not compute exact values. diff --git a/src/session.ts b/src/session.ts index 08ecd72..009e3f4 100644 --- a/src/session.ts +++ b/src/session.ts @@ -367,7 +367,7 @@ export class Session extends EventEmitter { source: opts.source, meta: opts.meta, hash: parentsValue == null ? undefined : this.graph.hashOf(nextDoc), - ...(parentsValue == null ? { inexact: true } : {}), + ...this.inexactMarker(parentsValue), ...this.recordMerge(parents, parentsValue), }; this.graph.add([envelope]); @@ -406,6 +406,15 @@ export class Session extends EventEmitter { return best == null ? {} : { mergeParent: best.parent, mergePatch: best.patch }; } + // A patch committed without the exact value of its parents by a session + // that computes exact values (e.g. before the history needed for it is + // loaded) is marked inexact (see Patch.inexact). A session that never + // computes exact values (no merge3, or apply-all) writes history the way + // patchflow 0.8 did, and its patches are read that way: no marker. + private inexactMarker(parentsValue: Document | undefined): Pick { + return parentsValue == null && this.graph.computesExactValues() ? { inexact: true } : {}; + } + // Merge a remote patch and refresh the current document. applyRemote(env: PatchEnvelope): void { this.applyRemoteBatch([env]); @@ -744,7 +753,7 @@ export class Session extends EventEmitter { version: nextVersion, file: true, hash: parentsValue == null ? undefined : this.graph.hashOf(newDoc), - ...(parentsValue == null ? { inexact: true } : {}), + ...this.inexactMarker(parentsValue), ...this.recordMerge(parents, parentsValue), }; this.graph.add([envelope]);