-
Notifications
You must be signed in to change notification settings - Fork 1
Partial parent handling + determinism + tests #1
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
base: master
Are you sure you want to change the base?
Conversation
|
@heureka91 This works correctly for insertions but fails in some deletion cases. Concurrent delete + insert loses both operations (expected "AXC", got "ABC"), and concurrent deletes at the same position delete twice instead of being idempotent (expected "ACD", got "AD"): /// Concurrent delete + insert - BUG: both operations lost
#[test]
fn delete_with_concurrent_insert() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "ABC")]));
graph.add_node(1, getOpList([TestOp::Del(2, -1)]), vec![0]); // Delete B
graph.add_node(2, getOpList([TestOp::Ins(1, "X")]), vec![0]); // Insert X
graph.add_node(3, empty_oplist(), vec![1, 2]);
let result = oplist_to_string(&graph.merge_graph());
assert_eq!(result, "AXC"); // FAILS: actual result is "ABC"
}
/// Concurrent deletes at same position - not idempotent
#[test]
fn delete_same_position_concurrent() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "ABCD")]));
graph.add_node(1, getOpList([TestOp::Del(2, -1)]), vec![0]); // Delete B
graph.add_node(2, getOpList([TestOp::Del(2, -1)]), vec![0]); // Delete B
graph.add_node(3, empty_oplist(), vec![1, 2]);
let result = oplist_to_string(&graph.merge_graph());
assert_eq!(result, "ACD"); // FAILS: actual result is "AD" (B and C both deleted)
} |
|
Thanks for the repro. I pushed a fix that handles concurrent deletes correctly by OT-transforming delete-containing ops against previously-applied siblings (same parent-set), so deletes are idempotent and don’t delete concurrent inserts. Added tests:
All tests passing locally: |
|
Found via fuzz testing against Fugue-Simple (reference implementation). These bugs appear after complex multi-node DAG scenarios with interleaved inserts, deletes, and merges. Bug 1: Delete fails after multi-branch merge with prior deletesAfter merging multiple branches that each contain delete operations, a subsequent delete on the merged result has no effect. The delete node is added to the graph, but /// Delete fails after complex merge history - BUG: delete has no effect
#[test]
fn delete_after_multi_branch_merge() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "AB")]));
// Branch 1: delete at pos 1
graph.add_node(1, getOpList([TestOp::Del(2, -1)]), vec![0]); // Delete B -> "A"
// Branch 2: insert then delete
graph.add_node(2, getOpList([TestOp::Ins(2, "C")]), vec![0]); // Insert C -> "ABC"
graph.add_node(3, getOpList([TestOp::Del(2, -1)]), vec![2]); // Delete B -> "AC"
// Merge branches
graph.add_node(4, empty_oplist(), vec![1, 3]);
// Add more content
graph.add_node(5, getOpList([TestOp::Ins(1, "X")]), vec![4]); // Insert X
// This delete should work but doesn't
graph.add_node(6, getOpList([TestOp::Del(1, -1)]), vec![5]); // Delete X
let result = oplist_to_string(&graph.merge_graph());
assert_eq!(result, "AC"); // FAILS: actual result still contains X
}Bug 2: Delete not applied when graph has deep merge ancestryWhen a graph has multiple levels of merges with deletes at each level, subsequent deletes fail to take effect: /// Delete fails with deep merge ancestry - BUG: delete ignored
#[test]
fn delete_with_deep_merge_ancestry() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "ABCD")]));
// First round: concurrent deletes
graph.add_node(1, getOpList([TestOp::Del(2, -1)]), vec![0]); // Delete B
graph.add_node(2, getOpList([TestOp::Del(3, -1)]), vec![0]); // Delete C
graph.add_node(3, empty_oplist(), vec![1, 2]); // Merge -> "AD"
// Second round: more operations
graph.add_node(4, getOpList([TestOp::Ins(1, "X")]), vec![3]); // Insert X -> "AXD"
graph.add_node(5, getOpList([TestOp::Ins(2, "Y")]), vec![3]); // Insert Y -> "AYD"
graph.add_node(6, empty_oplist(), vec![4, 5]); // Merge -> "AXYD" or "AYXD"
// This delete should remove one char but may not work
graph.add_node(7, getOpList([TestOp::Del(2, -1)]), vec![6]); // Delete at pos 1
let result = oplist_to_string(&graph.merge_graph());
// Expected: one character deleted from the merged result
// Actual: delete may have no effect
}Bug 3: Divergent results between synced replicasWhen simulating multi-replica sync (same operations applied in different orders), Mako produces different results than Fugue-Simple. After full sync, Mako may have extra characters that should have been deleted: /// Multi-replica sync divergence - BUG: Mako has extra chars
#[test]
fn multi_replica_sync_divergence() {
// Simulates 3 replicas with interleaved ops
// Replica 0: root
// Replica 1: inserts N, then N again, various deletes
// Replica 2: inserts P, deletes
// After full sync:
// Fugue-Simple: "O" (correct)
// Mako: "OB" (BUG - B should have been deleted)
} |
|
Pushed a fix for the fuzzer-reported delete issue.
All tests passing locally: Bug 3 (multi-replica divergence): if you can share a minimal concrete repro / expected string (from fugue-simple), I can add a test and iterate. |
Fuzz Testing Results After Commit 4c67a2cFound via fuzz testing against Fugue-Simple (reference implementation). The ancestor bitset fix in this commit addresses some delete transformation issues, but additional bugs remain in complex multi-replica scenarios. Bug 1: Concurrent insert ordering differs from FugueWhen concurrent inserts happen at the same position across different replicas, Mako and Fugue produce different orderings. This causes subsequent deletes to target wrong characters: /// Concurrent insert ordering differs from Fugue - BUG: wrong order
/// Expected: 'VE' (matching Fugue)
/// Actual: 'FV' (different ordering, then different char deleted)
#[test]
fn concurrent_insert_ordering_divergence() {
let mut graph = Graph::new(0, empty_oplist());
// Site 1 inserts 'V' at position 0
graph.add_node(1_000_001, getOpList([TestOp::Ins(0, "V")]), vec![0]);
// Site 0 sees V, inserts 'E' at position 1
graph.add_node(1, getOpList([TestOp::Ins(1, "E")]), vec![1_000_001]);
// Site 2 inserts 'F' at position 0 (concurrent with E, has not seen V yet)
graph.add_node(2_000_001, getOpList([TestOp::Ins(0, "F")]), vec![0]);
// Merge: site 2 receives all updates
graph.add_node(2_000_002, empty_oplist(), vec![1, 2_000_001]);
// Delete at position 1
graph.add_node(2_000_003, getOpList([TestOp::Del(2, -1)]), vec![2_000_002]);
let result = oplist_to_string(&graph.merge_graph());
// Fugue produces 'VE' (F deleted at pos 1 in 'VEF')
// Mako produces 'FV' (E deleted at pos 1 in 'FVE')
assert_eq!(result, "VE"); // FAILS: Mako has different insert ordering
}Bug 2: Delete incorrectly removes concurrent insert after syncAfter multiple syncs with concurrent operations, a character that was never explicitly deleted disappears from Mako's result: /// Delete removes wrong character after complex sync - BUG
/// Expected: 'HII' (matching Fugue - H was never deleted)
/// Actual: 'II' (H incorrectly removed)
#[test]
fn delete_removes_concurrent_insert_after_sync() {
let mut graph = Graph::new(0, empty_oplist());
// Site 3 inserts 'O', then 'X'
graph.add_node(3_000_001, getOpList([TestOp::Ins(0, "O")]), vec![0]);
graph.add_node(3_000_002, getOpList([TestOp::Ins(1, "X")]), vec![3_000_001]);
// Sync all - merge node
graph.add_node(100, empty_oplist(), vec![3_000_002]);
// Site 2 inserts 'T', site 0 deletes X, site 3 inserts 'H'
graph.add_node(2_000_001, getOpList([TestOp::Ins(2, "T")]), vec![100]);
graph.add_node(1, getOpList([TestOp::Del(2, -1)]), vec![100]); // Delete X
graph.add_node(3_000_003, getOpList([TestOp::Ins(2, "H")]), vec![100]);
// Sync all -> should have 'OTH'
graph.add_node(101, empty_oplist(), vec![2_000_001, 1, 3_000_003]);
// More concurrent operations
graph.add_node(3_000_004, getOpList([TestOp::Ins(3, "I")]), vec![101]);
graph.add_node(1_000_001, getOpList([TestOp::Del(1, -1)]), vec![101]); // Delete O
graph.add_node(2_000_002, getOpList([TestOp::Ins(3, "I")]), vec![101]);
graph.add_node(1_000_002, getOpList([TestOp::Del(1, -1)]), vec![1_000_001]); // Delete T
graph.add_node(2, getOpList([TestOp::Del(1, -1)]), vec![101]); // Delete O from site 0's view
// Final merge
graph.add_node(102, empty_oplist(), vec![3_000_004, 1_000_002, 2_000_002, 2]);
let result = oplist_to_string(&graph.merge_graph());
// H was inserted by site 3 and never explicitly deleted
// Fugue: 'HII', Mako: 'II'
assert_eq!(result, "HII"); // FAILS: H is incorrectly deleted
}Bug 3: Insert ordering reversed for concurrent operationsWhen two sites insert at the same position concurrently, the final ordering is reversed compared to Fugue: /// Concurrent inserts at same position have reversed order - BUG
/// Expected: 'IV' (matching Fugue)
/// Actual: 'VI' (reversed)
#[test]
fn concurrent_insert_order_reversed() {
let mut graph = Graph::new(0, empty_oplist());
// Two concurrent inserts at position 0
graph.add_node(1_000_001, getOpList([TestOp::Ins(0, "V")]), vec![0]); // Site 1
graph.add_node(2_000_001, getOpList([TestOp::Ins(0, "I")]), vec![0]); // Site 2
// Merge
graph.add_node(100, empty_oplist(), vec![1_000_001, 2_000_001]);
let result = oplist_to_string(&graph.merge_graph());
// Fugue orders by replica ID: site 1 < site 2, so 'V' before 'I' -> but position 0 means prepend
// The expected behavior depends on Fugue's tie-breaking rule
assert_eq!(result, "IV"); // FAILS: Mako produces 'VI'
}Bug 4: Extra character remains after delete should have removed itA delete operation fails to take effect, leaving an extra character in the result: /// Delete fails to remove character - BUG: extra char remains
/// Expected: 'S' (matching Fugue)
/// Actual: 'SS' (delete had no effect)
#[test]
fn delete_fails_extra_char_remains() {
let mut graph = Graph::new(0, empty_oplist());
// Insert 'S' twice from different sites
graph.add_node(1_000_001, getOpList([TestOp::Ins(0, "S")]), vec![0]);
graph.add_node(2_000_001, getOpList([TestOp::Ins(0, "S")]), vec![0]);
// Merge
graph.add_node(100, empty_oplist(), vec![1_000_001, 2_000_001]);
// Delete one S
graph.add_node(1_000_002, getOpList([TestOp::Del(1, -1)]), vec![100]);
let result = oplist_to_string(&graph.merge_graph());
// Should have 'S' after deleting one
assert_eq!(result, "S"); // FAILS: Mako still has 'SS'
}AnalysisThe bugs fall into two categories:
|
- Restructure as workspace with mako-rs, fugue-simple, and fuzz crates - Add Fugue-Simple CRDT implementation as ground truth reference - Add fuzz harness comparing Mako vs Fugue-Simple - Apply PR #1 fix: ancestor bitset for delete transforms - Add tests documenting remaining divergence bugs Remaining issues found: - Concurrent insert ordering differs from Fugue - Delete position drift in complex multi-replica scenarios
|
Separate branch for the fuzz testing code, in case it's relevant for further testing without my input: https://github.com/bxff/mako/tree/fuzz-testing-bugs |
|
Addressed the remaining ?concurrent insert ordering? bug from fuzz feedback.
Added/updated regression tests:
On the ?IV vs VI? note: I verified via Fugue-Simple?s own harness that two concurrent inserts at pos 0 with replica IDs 1 < 2 yields All tests passing locally: |
/// Hierarchical insert ordering test - KNOWN FAILING
/// This test documents the divergence between Mako and Fugue for hierarchical inserts.
///
/// Scenario (from fuzz seed 0xe91cd15e00000020):
/// - Site 0 inserts 'U' at pos 0, then 'R' at pos 1 (R is "child" of U)
/// - Site 1 inserts 'O' at pos 0 (concurrent with U)
/// - Site 2 inserts 'I' at pos 0, then 'H' at pos 1 (concurrent with U, O)
///
/// Expected (Fugue): 'UROIH' - R stays immediately after U
/// Actual (Mako): 'UOIHR' - R shifts to the end
///
/// Root cause: Mako transforms R against ALL non-ancestors (I, H, O),
/// but R should only be shifted by ops concurrent with its insertion context.
#[test]
#[ignore] // Remove when hierarchical insert ordering is fixed
fn hierarchical_insert_ordering_fugue_match() {
let mut graph = Graph::new(0, empty_oplist());
// Site 0: U at pos 0
graph.add_node(1_000_001, getOpList([TestOp::Ins(0, "U")]), vec![0]);
// Site 1: O at pos 0 (concurrent with U)
graph.add_node(2_000_001, getOpList([TestOp::Ins(0, "O")]), vec![0]);
// Site 2: I at pos 0 (concurrent with U, O)
graph.add_node(3_000_001, getOpList([TestOp::Ins(0, "I")]), vec![0]);
// Site 2: H at pos 1 (child of I)
graph.add_node(3_000_002, getOpList([TestOp::Ins(1, "H")]), vec![3_000_001]);
// Site 0: R at pos 1 (child of U)
graph.add_node(1_000_002, getOpList([TestOp::Ins(1, "R")]), vec![1_000_001]);
// Merge all (simulate SyncAll)
graph.add_node(100, empty_oplist(), vec![1_000_002, 2_000_001, 3_000_002]);
let result = oplist_to_string(&graph.merge_graph());
// Fugue produces 'UROIH' - R stays with U
// Mako currently produces 'UOIHR' - R shifts to end
assert_eq!(
result, "UROIH",
"Hierarchical inserts should match Fugue ordering. Got: {}",
result
);
} |
|
Also add the codex chat export if possible |
|
Pushed a fix for the hierarchical insert ordering repro + added a (best-effort) Codex log.
Latest commit: 2d6f853 |
|
@heureka91 this still fails /// ABCD deletion scenario - test if bug exists
/// Fugue-Simple: 'AXCD' - correct character deleted
/// Mako (bug): 'AXBC' - wrong character deleted
#[test]
fn fugue_abcd_deletion_scenario() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "A")]));
graph.add_node(1, getOpList([TestOp::Ins(0, "B")]), vec![0]);
graph.add_node(2, getOpList([TestOp::Ins(0, "C")]), vec![0]);
graph.add_node(3, getOpList([TestOp::Ins(0, "D")]), vec![0]);
graph.add_node(4, getOpList([TestOp::Del(1, -1)]), vec![1]);
graph.add_node(5, getOpList([TestOp::Ins(1, "X")]), vec![0, 2]);
graph.add_node(6, empty_oplist(), vec![3, 4, 5]);
let result = oplist_to_string(&graph.merge_graph());
// Fugue: 'AXCD', Mako: 'AXBC'
assert_eq!(
result, "AXCD",
"Should match Fugue. Mako produced: {}",
result
);
} |
|
you can export the codex chat using |
IGNORE, this was incorrectly flagged! My bad /// Complex sync delete scenario - KNOWN BUG
///
/// Start with 'B', add concurrent inserts X and Y, then merge and delete.
///
/// Fugue-Simple: 'XY' - B correctly deleted
/// Mako: 'BY' - wrong character deleted (X deleted instead of B)
#[test]
fn test_complex_sync_delete_scenario() {
let mut graph = Graph::new(0, getOpList([TestOp::Ins(0, "B")]));
graph.add_node(1, getOpList([TestOp::Ins(0, "X")]), vec![0]); // XB
graph.add_node(2, getOpList([TestOp::Ins(1, "Y")]), vec![0]); // BY
graph.add_node(3, empty_oplist(), vec![1, 2]); // Merge -> XBY
graph.add_node(4, getOpList([TestOp::Del(1, -1)]), vec![3]); // Delete at pos 0
let result = oplist_to_string(&graph.merge_graph());
// Fugue: 'XY' (B deleted), Mako: 'BY' (X deleted instead)
assert_eq!(
result, "XY",
"Should match Fugue. Mako produced: {}",
result
);
} |
This PR adds basic support for DAGs with partial-parent relationships and strengthens test coverage.
Changes:
pending_children.find_partial_parents_and_childrenhelper (+ unit tests) for discovering partial-parent frontiers.fugue-simpleground-truth ordering test (Figure 7 interleaving): expected final orderAXYBC.empty_oplist-only graphs).All tests pass:
cargo test.