Skip to content

Change CRDT logic to use tombstoning instead of ad-hoc resurrection - #4636

Merged
TrueDoctor merged 1 commit into
masterfrom
graph-storage-2-lww-existence
Oct 7, 2026
Merged

TrueDoctor merged 1 commit into
masterfrom
graph-storage-2-lww-existence

Conversation

@TrueDoctor

Copy link
Copy Markdown
Member

No description provided.

@Keavon Keavon changed the title Change crdt logic to use tombstoning instead of ad hoc ressurection Change CRDT logic to use tombstoning instead of ad-hoc resurrection Oct 2, 2026
@Keavon

Keavon commented Oct 2, 2026

Copy link
Copy Markdown
Member

Just in time for spooky season! 🪦🧟

@TrueDoctor
TrueDoctor changed the base branch from master to graph-storage-1-users October 2, 2026 17:49

@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.

3 issues found across 24 files

Confidence score: 2/5

  • attributes.rs makes existing Postcard registry snapshots fail to deserialize; #[serde(default)] does not preserve compatibility with positional binary encoding. Add a legacy decoder or migration.
  • to_runtime.rs can revive a deleted nested network on the next commit when a live owner still references it. Preserve the tombstone when staging from runtime.
  • staging_and_reopen.rs never reopens the document, so it does not verify the stated reopen behavior. Reopen through into_storage/open_in and assert the registry matches.
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="document/format/tests/staging_and_reopen.rs">

<violation number="1" location="document/format/tests/staging_and_reopen.rs:39">
P2: This test never reopens the document, so it only verifies in-memory staging and cannot validate the stated reopen behavior. Move the storage through `into_storage`/`open_in` and assert the reopened registry matches the staged batch.</violation>
</file>

<file name="document/graph-storage/src/to_runtime.rs">

<violation number="1" location="document/graph-storage/src/to_runtime.rs:177">
P1: This renders a deleted nested network whenever a live owner still references it. `stage_from_runtime` converts that runtime network back to a live entry and emits `AddNetwork`, reviving the tombstone on the next commit; preserve the removed state across this round trip.</violation>
</file>

<file name="document/graph-storage/src/attributes.rs">

<violation number="1" location="document/graph-storage/src/attributes.rs:18">
P1: Adding this positional field makes existing Postcard registry snapshots fail to deserialize; `#[serde(default)]` does not make the binary struct encoding backward-compatible. Add an explicit legacy decoder or migration before changing this persisted shape.</violation>
</file>

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

Re-trigger cubic

Comment thread document/graph-storage/src/resources.rs
Comment thread document/graph-storage/src/resources.rs
Comment thread document/graph-storage/src/registry.rs
Comment thread document/graph-storage/src/document.rs
Comment thread document/graph-storage/src/model.rs
Comment thread document/format/tests/staging_and_reopen.rs
Comment thread document/graph-storage/src/registry.rs
Comment thread document/graph-storage/src/attributes.rs

@timon-schelling timon-schelling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My main concern is having tombstones in the registry when saving without history. We could put them in a separate path right?

see the rest of the comments, in general LGTM

Comment thread document/graph-storage/src/attributes.rs Outdated
Comment thread document/graph-storage/src/attributes.rs Outdated
Comment thread document/graph-storage/src/registry.rs
Comment thread node-graph/rfcs/document-format.md
Comment thread document/graph-storage/src/registry.rs
@TrueDoctor
TrueDoctor force-pushed the graph-storage-2-lww-existence branch from 8af650b to 3ac7aa7 Compare October 5, 2026 20:40
Base automatically changed from graph-storage-1-users to master October 5, 2026 21:07
@TrueDoctor
TrueDoctor force-pushed the graph-storage-2-lww-existence branch from 3ac7aa7 to db10f2b Compare October 5, 2026 21:07

@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 existing issue remains and no new issues found across 21 files

Confidence score: 2/5

  • document/graph-storage/src/attributes.rs changes the positional Attributes encoding without changing how existing v1 registries are decoded, so stored data may fail to decode or be read incorrectly. Add a versioned decoder or migration before changing this encoding.

Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.

Re-trigger cubic

Comment thread node-graph/rfcs/document-format.md Outdated
Comment thread document/graph-storage/src/registry.rs
Comment thread document/graph-storage/src/attributes.rs
Comment thread document/graph-storage/src/to_runtime.rs
Comment thread document/graph-storage/src/document.rs
@TrueDoctor
TrueDoctor force-pushed the graph-storage-2-lww-existence branch from db10f2b to 60cca98 Compare October 7, 2026 15:54
@TrueDoctor
TrueDoctor force-pushed the graph-storage-2-lww-existence branch from 60cca98 to 4e1da8f Compare October 7, 2026 15:57
@TrueDoctor
TrueDoctor added this pull request to the merge queue Oct 7, 2026
Merged via the queue into master with commit 275d3d0 Oct 7, 2026
11 checks passed
@TrueDoctor
TrueDoctor deleted the graph-storage-2-lww-existence branch October 7, 2026 16:20
@Keavon Keavon mentioned this pull request Oct 10, 2026
6 of 51 tasks
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.

3 participants