Skip to main content

max / synckit

Delete the second implementation of Hlc's ordering cmp_hlc and max_hlc in store/hlc.rs re-derived the comparison that Hlc now states directly. The two agreed only because DeviceId derives Ord over its Uuid, and nothing tested that they agreed, so reordering Hlc's fields would have split them: collapse_max_hlc would pick a different winner than the conflict resolver and two devices would keep different rows. Three call sites now use Hlc's own Ord: the committed-ledger guard in set_committed, the Merged arm of resolve_pull, and collapse_max_hlc. The guard at set_committed was a third caller the write-up did not list. Probing the two rewritten comparisons found collapse_max_hlc untested: inverting it, so the pipeline keeps the oldest change per row rather than the newest, passed all 572 tests. Three tests now cover it (highest HLC per row in both arrival orders, operation-agnostic, per-row with first-seen order preserved), each verified to fail on the inverted comparison.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-05 15:41 UTC
Signed with PGP, not checked
Commit: 88fd9b77b829b40c947abe872a497d6486dc8560
Parent: 53f0b35
1 file changed, +80 insertions, -11 deletions
M synckit-client/src/store/hlc.rs +80 -11
@@ -126,7 +126,7 @@
126 126 /// Record a row's committed HLC, advancing only (never regress the ledger).
127 127 pub fn set_committed(conn: &Connection, table: &str, row_id: &str, hlc: &Hlc) -> Result<()> {
128 128 if let Some(existing) = committed_hlc(conn, table, row_id)?
129 - && cmp_hlc(&existing, hlc).is_ge()
129 + && existing >= *hlc
130 130 {
131 131 return Ok(());
132 132 }
@@ -234,7 +234,7 @@
234 234 }
235 235 // A merge keeps both sides' fields, so nothing was thrown away.
236 236 Resolution::Merged(data) => {
237 - let hlc = max_hlc(pair.local.hlc, pair.remote.entry.hlc);
237 + let hlc = pair.local.hlc.max(pair.remote.entry.hlc);
238 238 resolved.push(ChangeEntry {
239 239 table: pair.remote.entry.table,
240 240 op: ChangeOp::Update,
@@ -344,7 +344,7 @@
344 344 for e in entries {
345 345 let key = (e.table.clone(), e.row_id.clone());
346 346 match best.get(&key) {
347 - Some(&i) if cmp_hlc(&kept[i].hlc, &e.hlc).is_ge() => {}
347 + Some(&i) if kept[i].hlc >= e.hlc => {}
348 348 Some(&i) => kept[i] = e,
349 349 None => {
350 350 best.insert(key, kept.len());
@@ -355,14 +355,6 @@
355 355 kept
356 356 }
357 357
358 - fn cmp_hlc(a: &Hlc, b: &Hlc) -> std::cmp::Ordering {
359 - (a.wall_ms, a.counter, a.node.as_uuid()).cmp(&(b.wall_ms, b.counter, b.node.as_uuid()))
360 - }
361 -
362 - fn max_hlc(a: Hlc, b: Hlc) -> Hlc {
363 - if cmp_hlc(&a, &b).is_ge() { a } else { b }
364 - }
365 -
366 358 #[cfg(test)]
367 359 mod tests {
368 360 use super::super::apply::apply_remote_changes;
@@ -423,6 +415,83 @@
423 415 }
424 416 }
425 417
418 + /// A bare entry for the collapse tests: only table, row_id, op and HLC
419 + /// matter there, so the payload names the entry for the assertion message.
420 + fn entry(row_id: &str, op: ChangeOp, wall_ms: i64, node_n: u128, label: &str) -> ChangeEntry {
421 + ChangeEntry {
422 + table: "note".into(),
423 + op,
424 + row_id: row_id.into(),
425 + timestamp: Utc::now(),
426 + hlc: Hlc {
427 + wall_ms,
428 + counter: 0,
429 + node: node(node_n),
430 + },
431 + data: Some(serde_json::json!({ "name": label })),
432 + extra: serde_json::Map::default(),
433 + }
434 + }
435 +
436 + fn labels(entries: &[ChangeEntry]) -> Vec<String> {
437 + entries
438 + .iter()
439 + .map(|e| e.data.as_ref().unwrap()["name"].as_str().unwrap().into())
440 + .collect()
441 + }
442 +
443 + /// The collapse keeps the highest HLC per row, and it has to do so whichever
444 + /// order the entries arrive in. Both directions are asserted because the
445 + /// obvious way to get this wrong, comparing in the wrong direction, is
446 + /// invisible when only the already-sorted order is tested: it then keeps the
447 + /// last entry, which is also the newest.
448 + #[test]
449 + fn collapse_keeps_the_highest_hlc_per_row_in_either_order() {
450 + let older = entry("r1", ChangeOp::Update, 100, 1, "older");
451 + let newer = entry("r1", ChangeOp::Update, 200, 1, "newer");
452 +
453 + let ascending = collapse_max_hlc(vec![older.clone(), newer.clone()]);
454 + assert_eq!(labels(&ascending), ["newer"], "newest lost, arriving last");
455 +
456 + let descending = collapse_max_hlc(vec![newer, older]);
457 + assert_eq!(
458 + labels(&descending),
459 + ["newer"],
460 + "newest lost, arriving first"
461 + );
462 + }
463 +
464 + /// Operation-agnostic: a newer delete beats an older edit and an older
465 + /// delete loses to a newer edit. The HLC decides, never the operation.
466 + #[test]
467 + fn collapse_ignores_the_operation() {
468 + let newer_delete = collapse_max_hlc(vec![
469 + entry("r1", ChangeOp::Update, 100, 1, "edit"),
470 + entry("r1", ChangeOp::Delete, 200, 1, "delete"),
471 + ]);
472 + assert_eq!(newer_delete.len(), 1);
473 + assert_eq!(newer_delete[0].op, ChangeOp::Delete);
474 +
475 + let older_delete = collapse_max_hlc(vec![
476 + entry("r1", ChangeOp::Delete, 100, 1, "delete"),
477 + entry("r1", ChangeOp::Update, 200, 1, "edit"),
478 + ]);
479 + assert_eq!(older_delete.len(), 1);
480 + assert_eq!(older_delete[0].op, ChangeOp::Update);
481 + }
482 +
483 + /// The collapse is per row: distinct rows all survive, and first-seen order
484 + /// is preserved, which is what the doc comment promises the apply engine.
485 + #[test]
486 + fn collapse_is_per_row_and_keeps_first_seen_order() {
487 + let out = collapse_max_hlc(vec![
488 + entry("r2", ChangeOp::Update, 100, 1, "r2-old"),
489 + entry("r1", ChangeOp::Update, 100, 1, "r1-only"),
490 + entry("r2", ChangeOp::Update, 200, 1, "r2-new"),
491 + ]);
492 + assert_eq!(labels(&out), ["r2-new", "r1-only"]);
493 + }
494 +
426 495 fn note_name(conn: &Connection, id: &str) -> Option<String> {
427 496 conn.query_row("SELECT name FROM note WHERE id = ?1", [id], |r| r.get(0))
428 497 .optional()