Skip to content

Fix multi user undo model - #4639

Merged
TrueDoctor merged 2 commits into
masterfrom
graph-storage-5-exact-undo
Oct 15, 2026
Merged

TrueDoctor merged 2 commits into
masterfrom
graph-storage-5-exact-undo

Conversation

@TrueDoctor

Copy link
Copy Markdown
Member

No description provided.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread document/graph-storage/src/crdt.rs
Comment thread document/graph-storage/src/document.rs
Comment thread document/graph-storage/src/session.rs
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from edbac74 to 7f1228b Compare October 2, 2026 21:19
@TrueDoctor
TrueDoctor added this pull request to stack #4643 October 3, 2026 14:04
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from 7f1228b to 057cf3e Compare October 5, 2026 20:40
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from 057cf3e to 208c86a Compare October 5, 2026 21:07
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch 2 times, most recently from 1fac00e to c91252f Compare October 7, 2026 15:57
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from c91252f to 891e38f Compare October 7, 2026 16:22
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from 891e38f to 8a0805b Compare October 7, 2026 18:04
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from 8a0805b to b84da17 Compare October 7, 2026 18:54
@TrueDoctor
TrueDoctor force-pushed the graph-storage-5-exact-undo branch from 84cd9a4 to 7686d6e Compare October 14, 2026 11:03

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found and verified against the latest diff

Confidence score: 5/5

  • The O(steps moved) bound in node-graph/rfcs/document-format.md doesn’t hold while hot ops remain, because undo/redo also clones the snapshot and replays the hot log. Qualify the bound to account for that rebuild.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/rfcs/document-format.md">

<violation number="1" location="node-graph/rfcs/document-format.md:202">
P3: The unconditional O(steps moved) claim is false when hot ops remain: undo/redo rebuilds the working registry by cloning the snapshot and replaying the hot log. Qualify the bound to account for this rebuild.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

**Interactions, not deltas.** One user action diffs into several deltas (one per changed field, slot, or attribute), so undo steps per *interaction* rather than per delta. The last delta of each interaction is tagged with the `interaction_end` attribute, and undo reverts deltas walking the first-parent chain until the parent is an `interaction_end` boundary or the root. The starting `head` (the checkpoint) is pushed to the redo stack, and redo re-applies forward to it.
**Interactions, not deltas.** One user action diffs into several deltas (one per changed field, slot, or attribute), so undo steps per *interaction* rather than per delta. The last delta of each interaction is tagged with the `interaction_end` attribute, and undo reverts deltas walking the first-parent chain until the parent is an `interaction_end` boundary, a merge, another user's delta, or the root. The starting `head` (the checkpoint) is pushed to the redo stack, and redo re-applies forward to it.

**Restore, not reverse ops.** A delta's `reverse` is a list of `Prior`s: the state of every slot the op wrote, as it was just before, stamps included (a whole node, network, or resource where the op added, removed, or landed on a tombstone; otherwise one field plus the entity's presence stamp). Undo writes these back as they were, bypassing LWW, so the registry is again exactly the fold of history up to the new `head`. An op applied at its own timestamp could not express that: it can only write a value or a tombstone at that timestamp, while the prior state may be an older stamp or no entry at all, and a stamp the history does not account for decides later concurrent writes differently on different peers. Redo re-applies the forward ops with plain LWW, which folds them in exactly as retirement did. Both are O(steps moved), no clock advances, and identities are unchanged. Undo only takes this user's own interactions and never one containing a merge, since restoring past another peer's write would rewind it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The unconditional O(steps moved) claim is false when hot ops remain: undo/redo rebuilds the working registry by cloning the snapshot and replaying the hot log. Qualify the bound to account for this rebuild.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At node-graph/rfcs/document-format.md, line 202:

<comment>The unconditional O(steps moved) claim is false when hot ops remain: undo/redo rebuilds the working registry by cloning the snapshot and replaying the hot log. Qualify the bound to account for this rebuild.</comment>

<file context>
@@ -197,14 +197,11 @@ The silent zone is the implemented path (solo editing has no transport yet).
-**Interactions, not deltas.** One user action diffs into several deltas (one per changed field, slot, or attribute), so undo steps per *interaction* rather than per delta. The last delta of each interaction is tagged with the `interaction_end` attribute, and undo reverts deltas walking the first-parent chain until the parent is an `interaction_end` boundary or the root. The starting `head` (the checkpoint) is pushed to the redo stack, and redo re-applies forward to it.
+**Interactions, not deltas.** One user action diffs into several deltas (one per changed field, slot, or attribute), so undo steps per *interaction* rather than per delta. The last delta of each interaction is tagged with the `interaction_end` attribute, and undo reverts deltas walking the first-parent chain until the parent is an `interaction_end` boundary, a merge, another user's delta, or the root. The starting `head` (the checkpoint) is pushed to the redo stack, and redo re-applies forward to it.
 
+**Restore, not reverse ops.** A delta's `reverse` is a list of `Prior`s: the state of every slot the op wrote, as it was just before, stamps included (a whole node, network, or resource where the op added, removed, or landed on a tombstone; otherwise one field plus the entity's presence stamp). Undo writes these back as they were, bypassing LWW, so the registry is again exactly the fold of history up to the new `head`. An op applied at its own timestamp could not express that: it can only write a value or a tombstone at that timestamp, while the prior state may be an older stamp or no entry at all, and a stamp the history does not account for decides later concurrent writes differently on different peers. Redo re-applies the forward ops with plain LWW, which folds them in exactly as retirement did. Both are O(steps moved), no clock advances, and identities are unchanged. Undo only takes this user's own interactions and never one containing a merge, since restoring past another peer's write would rewind it.
 
-**Force-apply.** Rewinding re-applies each delta's precomputed `reverse` (for redo, the forward `kind`). These carry the *original* timestamp, which would tie (and so lose) the LWW arms' strict `>` comparison, since the forward op already stamped each field at that timestamp. In the single-writer silent zone the rewind value is authoritative, so silent undo/redo apply in a **force** mode where LWW arms assign unconditionally and structural ops are idempotent. Undo and redo are symmetric (force-reverse, force-forward), so no clock advances and identities are unchanged.
</file context>
Suggested change
**Restore, not reverse ops.** A delta's `reverse` is a list of `Prior`s: the state of every slot the op wrote, as it was just before, stamps included (a whole node, network, or resource where the op added, removed, or landed on a tombstone; otherwise one field plus the entity's presence stamp). Undo writes these back as they were, bypassing LWW, so the registry is again exactly the fold of history up to the new `head`. An op applied at its own timestamp could not express that: it can only write a value or a tombstone at that timestamp, while the prior state may be an older stamp or no entry at all, and a stamp the history does not account for decides later concurrent writes differently on different peers. Redo re-applies the forward ops with plain LWW, which folds them in exactly as retirement did. Both are O(steps moved), no clock advances, and identities are unchanged. Undo only takes this user's own interactions and never one containing a merge, since restoring past another peer's write would rewind it.
**Restore, not reverse ops.** A delta's `reverse` is a list of `Prior`s: the state of every slot the op wrote, as it was just before, stamps included (a whole node, network, or resource where the op added, removed, or landed on a tombstone; otherwise one field plus the entity's presence stamp). Undo writes these back as they were, bypassing LWW, so the registry is again exactly the fold of history up to the new `head`. An op applied at its own timestamp could not express that: it can only write a value or a tombstone at that timestamp, while the prior state may be an older stamp or no entry at all, and a stamp the history does not account for decides later concurrent writes differently on different peers. Redo re-applies the forward ops with plain LWW, which folds them in exactly as retirement did. With no hot ops, undo/redo cost scales with the steps moved; with hot ops, rebuilding the working registry also costs O(registry size + hot ops). No clock advances, and identities are unchanged. Undo only takes this user's own interactions and never one containing a merge, since restoring past another peer's write would rewind it.

@TrueDoctor
TrueDoctor added this pull request to the merge queue Oct 15, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 15, 2026
@TrueDoctor
TrueDoctor added this pull request to the merge queue Oct 15, 2026
Merged via the queue into master with commit 243a83c Oct 15, 2026
11 checks passed
@TrueDoctor
TrueDoctor deleted the graph-storage-5-exact-undo branch October 15, 2026 18:23
VimYoung pushed a commit to VimYoung/Graphite that referenced this pull request Oct 26, 2026
* Undo by restoring each step's exact prior state, and only this user's own steps

* Split up prior variants

This branch was successfully deployed

1 active deployment
graphite-dev (Preview) — 7686d6eb Deployed Oct 14, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants