Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for missing CGKA ops #85

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add test for missing CGKA ops #85

wants to merge 5 commits into from

Conversation

alexjg
Copy link
Member

@alexjg alexjg commented Mar 9, 2025

This PR is a test which exposes a bug which I'm running into with the CGKA ops. The problem occurs when we sync membership operations and prekey operations but not cgka operations to another peer who initially had no data. Calling Keyhive::cgka_ops_for_agent (or anything which ends up calling Document::cgka_ops) will panic in CgkaOperationGraph::topsort_for_heads because there is an assertion (on line 272 of keyhive_core/src/cgka/operation.rs) that the heads of the operation graph are not empty - but they are empty because we haven't interacted with the cgka graph at all yet.

@alexjg alexjg requested review from expede, ept and pvh as code owners March 9, 2025 18:49
.unwrap();

// Now pull out all the membership operations and prekey operations from alice
let mut events = alice.static_events_for_agent(&bob_indi.into()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Here's the relevant diff

-        let mut events = alice
-            .membership_ops_for_agent(&bob.active().clone().into())
-            .into_values()
-            .map(|v| StaticEvent::from(Event::from(v)))
-            .chain(
-                alice
-                    .reachable_prekey_ops_for_agent(&bob.active().clone().into())
-                    .into_iter()
-                    .flat_map(|(_, v)| {
-                        v.into_iter().map(|v| {
-                            StaticEvent::from(Event::<MemorySigner, _, NoListener>::from(
-                                Rc::unwrap_or_clone(v),
-                            ))
-                        })
-                    }),
-            )
-            .collect::<Vec<_>>();
+        let mut events = alice.static_events_for_agent(&bob_indi.into()).unwrap();

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue here is that I actually want to do the membership and prekeys ops in a separate step to the cgka ops in order to facilitate partial synchronizatipn and prioritization. I.e. I want to sync membership and prekeys ops at the beginning of sync, but then do things like prioritise the list of out of sync documents and sync cgka ops for higher priority documents first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's also always possible - even with the static_events_for_agent call - that we fall over half way through syncing ops and end up in the situation where we've processed the membership ops and prekeys ops but not the cgka ops so the cgka opgraph heads could legitimately be empty.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay. I think the problem was that you're not syncing CGKA ops here. Prekeys != Cgka

Copy link
Member

Choose a reason for hiding this comment

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

I have a couple quetsions about the sync strategy, but we'll talk live shortly :)

Comment on lines +1804 to +1805
// FIXME: Alex plz confirm no longer the case. Original comment as follows:
// This will throw an error because the heads of the cgka operation graph are empty
Copy link
Member

@expede expede Mar 10, 2025

Choose a reason for hiding this comment

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

I added an assert! below just to make the fix easier to confirm. If this is the behaviour that you expect, delete these comments!

Comment on lines +1780 to +1784
// Now pull out all the membership operations and prekey operations from alice
let events = alice.events_for_agent(&bob_indi.into()).unwrap();

// Now ingest all these events on bob
bob.ingest_event_table(events).unwrap(); // Test helper
Copy link
Member

@expede expede Mar 10, 2025

Choose a reason for hiding this comment

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

events_for_agent here is the same as static_events_for_agent, but without serialization. We use a helper below that can take unserialized events (though it actually serializes under the hood for code sharing reasons but it's more convenient this way IMO but lemme know if you disagree!)

I also switched to using an ingestion helper. ingest_event_table and its sibling ingest_unsorted_static_events are hidden behind a cfg(any(test, feature="test_utils") because they're not super efficient, but they're really helpful for test setup (like here). They run to a fixed point; if the fixed point is empty then you're Ok, but Err if you don't ingest any new ops between two cycles.

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