Skip to content

apply_diff applies part of a batch before returning an error (e.g. OutOfBound) #1154

Closed

Description

@zxch3n

Problem

LoroDoc::apply_diff (_apply_diff in crates/loro-internal/src/loro.rs) turns each diff of a batch into local ops one after another. When one diff fails, the error is recorded and the loop continues. Nothing rolls back the diffs applied before (or after) it, so the call returns Err and the doc has still changed. The same applies to revert_to and to JS applyDiff/revertTo.

#1142 handles this for one cause, diffs that would create a container of an unknown type, by checking the whole batch before anything is applied. Other invalid input still leaves the doc half-updated:

  • out-of-range positions (OutOfBound);
  • a diff made for another version of the doc;
  • a diff type that doesn't match its container.

Minimal reproduction

use loro::{event::{Diff, DiffBatch, ListDiffItem}, ContainerID, ContainerType, LoroDoc, TextDelta, ToJson};

let doc = LoroDoc::new();
doc.get_list("list").push(1).unwrap();
doc.commit();
let mut batch = DiffBatch::default();
batch.push(
    ContainerID::new_root("text", ContainerType::Text),
    Diff::Text(vec![TextDelta::Insert { insert: "partial".into(), attributes: None }]),
).unwrap();
batch.push(
    ContainerID::new_root("list", ContainerType::List),
    Diff::List(vec![ListDiffItem::Delete { delete: 5 }]),
).unwrap();
let r = doc.apply_diff(batch);
doc.commit();
// Err(OutOfBound { pos: 5, len: 1, .. }), yet the text was inserted:
// {"list":[1],"text":"partial"}
println!("{r:?} {}", doc.get_deep_value().to_json_value());

A random test in #1142 hit this in 61 of 200 seeds, with only known container types. It applied doc.diff(a, b) to a doc that was not at version a, so the positions were stale.

Possible directions

  • Validate positions and diff types for the whole batch before applying it. DiffBatch::validate_for_apply only checks for index overflow. The _apply_diff loop recreates containers and remaps ids as it goes, so this validation has to predict that; see the unknown-container pre-check in fix: reject creating unknown-type containers instead of panicking #1142.
  • Or add a real rollback for the local ops of the batch, since the txn is local and uncommitted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions