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

fix: tracker skip applied deletion error #512

Merged
merged 1 commit into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/fuzz/src/crdt_fuzzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,7 @@ where
#[allow(clippy::redundant_clone)]
let mut actions_clone = actions.clone();
let action_ref: usize = (&mut actions_clone) as *mut _ as usize;
eprintln!("Initializing");
#[allow(clippy::blocks_in_conditions)]
if std::panic::catch_unwind(|| {
// SAFETY: test
Expand All @@ -573,6 +574,7 @@ where
return;
}

eprintln!("Started!");
let minified = Arc::new(Mutex::new(actions.clone()));
let candidates = Arc::new(Mutex::new(VecDeque::new()));
println!("Setup candidates...");
Expand Down Expand Up @@ -644,7 +646,7 @@ where
}

for thread in threads.into_iter() {
thread.join().unwrap();
thread.join().unwrap_or_default();
}

let minified = normalize(site_num, &mut minified.try_lock().unwrap());
Expand Down
196 changes: 196 additions & 0 deletions crates/fuzz/tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12908,6 +12908,202 @@ fn movable_list_undo_impl_1() {
)
}

#[test]
fn movable_list_err() {
test_multi_sites(
5,
vec![FuzzTarget::All],
&mut [
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(MovableList),
bool: true,
key: 488447261,
pos: 2097865012304223517,
length: 876435809836317,
prop: 2097865012304216064,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(Unknown(226)),
bool: false,
key: 16850407,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097900196676341904,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: I32(797),
bool: false,
key: 488447261,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097865012304253072,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: I32(488447261),
bool: true,
key: 488447261,
pos: 1542381706525,
length: 10416835838496021789,
prop: 2097865012311789712,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(Unknown(226)),
bool: false,
key: 16850407,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097900196676341904,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(MovableList),
bool: true,
key: 488447261,
pos: 2097865012304223725,
length: 876435809836317,
prop: 2097865012304216064,
}),
},
Handle {
site: 29,
target: 201,
container: 201,
action: Generic(GenericAction {
value: I32(488447261),
bool: true,
key: 488447261,
pos: 2097865012304223517,
length: 2097865012421664029,
prop: 10416984888674156829,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(Unknown(226)),
bool: false,
key: 16850407,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097900196676341904,
}),
},
Handle {
site: 29,
target: 30,
container: 29,
action: Generic(GenericAction {
value: I32(797),
bool: false,
key: 488453429,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097865012304253072,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: I32(488447261),
bool: true,
key: 488447261,
pos: 1542381706525,
length: 10416835838496021789,
prop: 2097865012311789712,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: I32(488447261),
bool: true,
key: 488447261,
pos: 2097872708885617949,
length: 10416984306455747869,
prop: 2097865012304253072,
}),
},
Handle {
site: 29,
target: 29,
container: 1,
action: Generic(GenericAction {
value: I32(488447261),
bool: true,
key: 488447261,
pos: 2097865012304223517,
length: 2097865012421664029,
prop: 10416984888674156829,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: Container(Unknown(226)),
bool: false,
key: 16850407,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097900196676341904,
}),
},
Handle {
site: 29,
target: 29,
container: 29,
action: Generic(GenericAction {
value: I32(797),
bool: false,
key: 488447261,
pos: 2097865012304223517,
length: 10416984306455747869,
prop: 2097865012304253072,
}),
},
SyncAllUndo {
site: 144,
op_len: 488447376,
},
],
)
}

#[test]
fn minify() {
minify_error(
Expand Down
1 change: 1 addition & 0 deletions crates/loro-internal/src/container/list/list_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ impl HasLength for DeleteSpan {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
pub struct DeleteSpanWithId {
/// This is the target id with the smallest counter no matter whether the span is reversed.
/// So it's always the id of the leftmost element of the target span.
pub id_start: ID,
/// The deleted position span
pub span: DeleteSpan,
Expand Down
14 changes: 13 additions & 1 deletion crates/loro-internal/src/container/richtext/tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ impl Tracker {
/// If `reverse` is true, the deletion happens from the end of the range to the start.
/// So the first op is the one that deletes element at `pos+len-1`, the last op
/// is the one that deletes element at `pos`.
///
/// - op_id: the first op that perform the deletion
/// - target_start_id: in the target deleted span, it's the first id of the span
/// - pos: the start pos of the deletion in the target span
/// - len: the length of the deletion span
/// - reverse: if true, the kth op delete the last kth element of the span
pub(crate) fn delete(
&mut self,
mut op_id: ID,
Expand All @@ -165,7 +171,13 @@ impl Tracker {
// the op is partially included, need to slice the op
let start = (applied_counter_end - op_id.counter) as usize;
op_id.counter = applied_counter_end;
target_start_id = target_start_id.inc(start as i32);
if !reverse {
target_start_id = target_start_id.inc(start as i32);
}
// Okay, this looks pretty weird, but it's correct.
// If it's reverse, we don't need to change the target_start_id, because target_start_id always pointing towards the
// leftmost element of the span. After applying the initial part of the deletion, which starts from the right side,
// the target_start_id will be still pointing towards the same leftmost element, thus no need to change.
len -= start;
// If reverse, don't need to change the pos, because it's deleting backwards.
// If not reverse, we don't need to change the pos either, because the `start` chars after it are already deleted
Expand Down
Loading