UndoManager hides diff application errors and consumes failed history items #1186

Affected versions and current-main status

The following natural failure is reproduced on Rust loro 1.16.2
(af130d97). Its mergeable-revival trigger is fixed on main at
4f60d921d820de3522624a436de77f994a6622b8: both arrival orders now undo the
remote insertion successfully. Main still contains the warning-and-success
error suppression path for application errors other than the separately prechecked
unknown-container rejection. No remaining natural failure of that path on this main
commit is claimed here.

Minimal Rust reproduction

Create a new Rust crate and put the following code in tests/repro.rs.

[package]
name = "loro-undo-issue-repro"
version = "0.1.0"
edition = "2021"

[dependencies]
loro = "=1.16.2"
serde_json = "1"
tracing = "0.1"
tracing-subscriber = { version = "0.3", features = ["fmt"] }

Run cargo test -- --nocapture --test-threads=1. To test the referenced main
commit, replace the loro dependency with:

loro = { git = "https://github.com/loro-dev/loro", rev = "4f60d921d820de3522624a436de77f994a6622b8" }

The tests below characterize the observed results; passing them demonstrates
the reported behaviour, not that it is correct.

use loro::{ContainerTrait, ExportMode, LoroDoc, LoroMap, ToJson, UndoManager};
use serde_json::{json, Value};

fn pair(values: &[i32]) -> (LoroDoc, LoroDoc) {
    let a = LoroDoc::new(); a.set_peer_id(1).unwrap();
    for (i, x) in values.iter().enumerate() { a.get_movable_list("list").insert(i, *x).unwrap(); }
    a.commit();
    let b = LoroDoc::new(); b.set_peer_id(2).unwrap();
    send(&a, &b);
    (a, b)
}

fn send(a: &LoroDoc, b: &LoroDoc) { b.import_with(&a.export(ExportMode::all_updates()).unwrap(), "import").unwrap(); }

fn sync(a: &LoroDoc, b: &LoroDoc) { send(a,b); send(b,a); }

fn undo(a: &LoroDoc) -> UndoManager {
    let mut u = UndoManager::new(a); u.add_exclude_origin_prefix("import"); u.set_merge_interval(0); u
}

fn list(a: &LoroDoc) -> Value { a.get_movable_list("list").get_value().to_json_value() }

fn state(a: &LoroDoc) -> Value { a.get_deep_value().to_json_value() }

#[test]
fn case3_swallowed_apply_failure() {
    let subscriber=tracing_subscriber::fmt().with_max_level(tracing::Level::WARN).with_ansi(false).without_time().with_writer(std::io::stdout).finish();
    let _guard=tracing::subscriber::set_default(subscriber);
    for delete_arrives_first in [false,true] {
        let (a,b)=pair(&[]);
        let root=a.get_map("root"); let la=root.ensure_mergeable_movable_list("child").unwrap();
        la.insert(0,0).unwrap(); a.commit(); send(&a,&b);
        let lb=b.get_movable_list(la.id()); let mut ua=undo(&a); let mut ub=undo(&b);
        lb.insert(1,1).unwrap(); b.commit();
        root.delete("child").unwrap(); a.commit();
        if delete_arrives_first {send(&a,&b);}
        send(&b,&a); let ar=ua.undo(); send(&a,&b);
        let before=lb.get_value().to_json_value(); let count=ub.undo_count(); let br=ub.undo(); let after=lb.get_value().to_json_value();
        println!("CASE3 delete_arrives_first={delete_arrives_first} A.undo={ar:?} B.before={before} B.stack={count} B.undo={br:?} B.after={after} B.stack={}",ub.undo_count());
    }
}

Observed versus expected

When the parent deletion reaches B before A's undo reactivates the mergeable
child, 1.16.2 produces:

WARN loro_internal::loro: Undo Failed OutOfBound { pos: 4, len: 3, info: "apply_delta" }
B.before=[0,0,1] B.stack=1
B.undo=Ok(false) B.after=[0,0,1] B.stack=0

The failed application should be observable as an error, and a failed step
should not silently consume unrelated history. The duplicated base value in
this particular trigger is the mergeable revival defect addressed upstream by
#1134; it is not evidence that main still
reproduces the same failure.

Root cause pointers

• crates/loro-internal/src/loro.rs:1256 (1.16.2), crates/loro-internal/src/loro.rs:1434 (main): if let Err(e) suppresses _apply_diff errors; the warning is at lines 1257 / 1439 respectively.
• crates/loro-internal/src/loro.rs:1254 on 1.16.2: the comment explains that
  MovableList collaborative undo can fail in edge cases and deliberately
  ignores those failures.
• crates/loro-internal/src/handler/movable_list_apply_delta.rs:53 on 1.16.2:
  malformed/out-of-range positional deltas return LoroError::OutOfBound.
• crates/loro-internal/src/undo.rs:863,939 on 1.16.2: the manager receives an
  apparent success and treats a zero-operation result as a no-op, potentially
  proceeding to older items.
• crates/loro-internal/src/loro.rs:1398 on 1.16.2: _apply_diff remembers a
  handler error but continues applying other containers, so failure is not
  atomic.
• crates/loro-internal/src/undo.rs:879 on 1.16.2: the existing error branch
  restores an item with ordinary push, which can assign a fresh remote-diff
  row. A draft surfacing errors required preserving the original row to make
  an unchanged failed step retry consistently.

Main also has all-or-nothing application for public apply_diff / revert_to,
while undo still calls _apply_diff directly. This distinction is documented in
context/apply-diff-atomicity.md; its rollback machinery may be relevant to an
undo-specific solution. Unknown-container recreation is prechecked and returned
separately on main; the warning branch is not a claim that every undo error is
suppressed.

Failure contract question

Returning the error is necessary, but replacing the warning with ? alone is
insufficient. A mixed batch can revert a map key and then return an OutOfBound
from the list. In the investigation's error-surfacing draft this was verified:
other.edited=42 disappeared while root.child=[0,0,1] stayed unchanged.

Should undo be atomic, or return a structured partial-application outcome? If
partial application remains possible, stack restoration, transform bases,
container remapping and retry semantics need an explicit contract. A caller
must not infer that Err means no mutation.

This error-handling issue is separate from the bounded-step PR. The earlier
undo_collab_list_move test (crates/loro/tests/integration_test/undo_test.rs:1517,
#[ignore]) illustrates the broader MovableList undo limitations, but does not
exercise this error branch.

Related: #1183.