From 8490e79bbb2c1ca114a1cc8846cde066644d796c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 15 May 2026 22:04:42 -0400 Subject: [PATCH] M10.11 fixes --- src/daemon.rs | 115 +++++++++++++++++++++++-------------- src/optimistic.rs | 59 +++++++++++++++++++ tests/m10_11_acceptance.rs | 59 +++++++++++++++---- 3 files changed, 178 insertions(+), 55 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index f49da81..fbddb51 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -256,6 +256,33 @@ impl SplitMix64 { } } +/// T M10.11 F2 — apply jitter delay before a wire write. +/// +/// One *mechanism* (this fn), called at two *sites*: the `CrdtOp` +/// broadcast write (criterion 3 — the CRDT-convergence path) and the +/// render-message `CellDelta` write (criterion 1 — the render-latency +/// path). **Q6's original "no new injection seams; one place" +/// commitment was wrong** and Finding 5's first resolution compounded +/// the error: criterion 1 and criterion 3 ride *different message +/// paths* (render output vs `broadcast_crdt_op`), so they +/// structurally require two call sites. Honest framing: one jitter +/// mechanism, two call sites because there are two paths — not "one +/// seam" (there isn't) and not "widen one loop's match" (Finding 5's +/// flawed fix, which matched `CrdtOp` in the render loop that never +/// carries broadcast `CrdtOp`s). +/// +/// No-op when `jitter_ms == 0`. Tension-B holds: the write is +/// *delayed*, never dropped. +fn maybe_jitter_sleep(jitter_ms: u64, base_ms: u64, rng: &mut SplitMix64) { + if jitter_ms == 0 { + return; + } + let delay_ms = base_ms + (rng.next_u64() % jitter_ms); + if delay_ms > 0 { + thread::sleep(Duration::from_millis(delay_ms)); + } +} + /// T M10.8 Day 4 — RAII guard for the non-multi-session slot. /// /// Acquired via [`NonMultiSlotGuard::try_acquire`] at per-attach @@ -804,6 +831,22 @@ fn dispatcher_loop( let entries = session_registry.broadcast_crdt_op(exclude, buffer_id, op); for entry in entries { if let Some(stream) = streams.get_mut(&entry.recipient) { + // T M10.11 F2 — THE criterion-3 jitter site. + // CRDT convergence is driven by these + // `broadcast_crdt_op` writes, NOT by render + // CellDeltas. Finding 5's first fix jittered + // the render loop (which never carries + // broadcast CrdtOps) and falsely claimed + // criterion 3 was exercised. This is the + // write that actually delivers ops to + // replicas; jittering here is what makes + // `m10_11_q8_convergence_under_jitter` + // genuinely test CRDT-under-jitter. + maybe_jitter_sleep( + injected_render_latency_jitter_ms, + injected_render_latency_ms, + &mut jitter_rng, + ); let _ = write_message(stream, &entry.message); } } @@ -877,52 +920,36 @@ fn dispatcher_loop( let mut write_failed = false; if let Some(stream) = streams.get_mut(fid) { for msg in &messages { - // T M10.10 Day 4 — test-only latency injection. - // When `PMACS_INSTANCE_LATENCY_MS` is set (>0), - // sleep before each CellDelta write to simulate - // slow daemon→frontend transport. Verifies - // criterion 1 ("less than one frame regardless of - // instance latency") under realistic high-latency - // conditions. Dispatcher-wide scope: multi- - // frontend tests at injected latency conflate - // frontends. - // T M10.11 Q6/Q8 Finding 5 — two scoped modes, - // one seam (Q6's "no new injection seams" - // preserved: single sleep-site; the match scope, - // not the seam count, varies): + // T M10.10 Day 4 / M10.11 F2 — the criterion-1 + // jitter site: render-write latency. // - // - **Fixed-latency mode** (`PMACS_INSTANCE_LATENCY_MS`, - // no jitter): CellDelta-only, unchanged from - // M10.10 Day 4. Criterion 1 ("local edit visible - // in <1 frame regardless of instance latency") - // is a *render-write* latency property; CellDelta - // is the right and only target. Preserving this - // scope exactly keeps criterion-1 tests' behavior - // identical. - // - **Jitter mode** (`PMACS_INSTANCE_LATENCY_JITTER_MS`): - // CellDelta *and* CrdtOp. Criterion 3 ("the CRDT - // layer converges under jitter") lives on the - // CrdtOp path — CRDT convergence is CrdtOp-driven, - // not CellDelta. Finding 5: Q6's CellDelta-only - // scope did not exercise criterion 3's assertion - // target; widening jitter-mode to CrdtOp closes - // that composition gap. Tension-B holds — both - // message types are *delayed*, neither *dropped*. - if injected_render_latency_jitter_ms > 0 { - if matches!( - msg, - InstanceMessage::CellDelta { .. } | InstanceMessage::CrdtOp { .. } - ) { - let delay_ms = injected_render_latency_ms - + (jitter_rng.next_u64() % injected_render_latency_jitter_ms); - if delay_ms > 0 { - thread::sleep(Duration::from_millis(delay_ms)); - } + // `messages` is render output only (CellDelta / + // Cursor / CursorByte) — it NEVER carries broadcast + // CrdtOps (those go out via `broadcast_crdt_op` at + // the top of the loop, the criterion-3 site). So + // jitter here is CellDelta-only *by the nature of + // this loop*, not by a match choice. Finding 5's + // first fix added `| CrdtOp` to the match below + // believing it widened jitter to the CRDT path; + // that arm was dead — no broadcast CrdtOp ever + // reaches this loop. Reverted to honest + // CellDelta-only; criterion-3 jitter lives at the + // broadcast site via the same `maybe_jitter_sleep` + // mechanism. Criterion 1 ("local edit visible in + // <1 frame regardless of instance latency") is a + // render-write-latency property; CellDelta is its + // correct and only target. Fixed-latency mode + // (no jitter) is unchanged from M10.10 Day 4. + if matches!(msg, InstanceMessage::CellDelta { .. }) { + if injected_render_latency_jitter_ms > 0 { + maybe_jitter_sleep( + injected_render_latency_jitter_ms, + injected_render_latency_ms, + &mut jitter_rng, + ); + } else if injected_render_latency_ms > 0 { + thread::sleep(Duration::from_millis(injected_render_latency_ms)); } - } else if injected_render_latency_ms > 0 - && matches!(msg, InstanceMessage::CellDelta { .. }) - { - thread::sleep(Duration::from_millis(injected_render_latency_ms)); } if let Err(e) = write_message(stream, msg) { eprintln!("pmacs: write failed for {fid:?} in dispatcher: {e}"); diff --git a/src/optimistic.rs b/src/optimistic.rs index 7e4d78c..6b27d84 100644 --- a/src/optimistic.rs +++ b/src/optimistic.rs @@ -1151,4 +1151,63 @@ mod tests { "remote op must apply when source != local" ); } + + /// **F1 gap pin.** The manual checklist originally told operators + /// to undo with `C-x u`. That is the *wrong* keystroke for the + /// per-frontend optimistic-undo path Scenario 2 tests: only the + /// single-key forms (`Ctrl-4`, and — under Kitty enhanced mode — + /// `Ctrl-/` / `Ctrl-_`) classify as `OptimisticAction::Undo` + /// (frontend per-peer undo). `C-x` is a multi-key prefix the + /// optimistic layer has no state for; it classifies `RoundTrip` + /// and the sequence `C-x u` round-trips to the *daemon's* undo, + /// which operates on the daemon's CRDT peer and cannot isolate a + /// single frontend's edits. + /// + /// This pins the gap as a tested invariant rather than prose: + /// if a future change made `C-x` optimistic, or de-classified + /// `Ctrl-4`, this fails — and the checklist's `Ctrl-4` + /// instruction (F1 fix) would silently become wrong again. + #[test] + fn f1_undo_keystroke_gap_cx_u_round_trips_only_single_key_is_optimistic() { + // The keystroke the manual checklist (post-F1) and the PTY + // test both use — reaches frontend per-peer undo. + assert_eq!( + classify_key(Key::Char('4'), Modifiers::CTRL), + OptimisticAction::Undo, + "Ctrl-4 must be the frontend per-peer optimistic undo \ + (raw-terminal-deliverable; what the checklist now uses)" + ); + // Kitty-enhanced-mode forms — also optimistic undo (only + // delivered when Kitty negotiation lands; v0.2). + assert_eq!( + classify_key(Key::Char('/'), Modifiers::CTRL), + OptimisticAction::Undo + ); + assert_eq!( + classify_key(Key::Char('_'), Modifiers::CTRL), + OptimisticAction::Undo + ); + // `C-x` — the prefix of the OLD (wrong) checklist instruction + // `C-x u`. Round-trips; the optimistic layer has no multi-key + // prefix state, so `C-x u` can NEVER compose to frontend + // per-peer undo — it reaches daemon undo, which Scenario 2's + // per-frontend-isolation claim is not about. + assert_eq!( + classify_key(Key::Char('x'), Modifiers::CTRL), + OptimisticAction::RoundTrip, + "C-x must round-trip — it's the daemon-undo prefix, NOT \ + frontend per-peer undo; this is why the checklist had \ + to switch from C-x u to Ctrl-4 (F1)" + ); + // The lone `u` after `C-x`, seen in isolation by the + // stateless optimistic layer, is just text — confirming no + // prefix-composition path to undo exists. + assert_eq!( + classify_key(Key::Char('u'), Modifiers::NONE), + OptimisticAction::Insert('u'), + "no multi-key prefix state: the 'u' in C-x u is plain \ + text to the optimistic layer; C-x u cannot be \ + frontend-undo by construction" + ); + } } diff --git a/tests/m10_11_acceptance.rs b/tests/m10_11_acceptance.rs index a3423c8..9f9caf4 100644 --- a/tests/m10_11_acceptance.rs +++ b/tests/m10_11_acceptance.rs @@ -1161,22 +1161,33 @@ fn m10_11_q13_cat2_undo_across_delayed_ops() { /// Daemon spawned with `PMACS_INSTANCE_LATENCY_JITTER_MS=50` and a /// pinned seed (`0xC0FFEE` = 12648430) so the delay pattern is /// deterministically reproducible — a flake's seed is the one to -/// re-run (framing Q8). Per **Finding 5 + (B)**, jitter-mode delays -/// both `CellDelta` *and* `CrdtOp`, so the CRDT-convergence path -/// (which is CrdtOp-driven, not CellDelta) is actually exercised -/// under jitter — criterion 3 says "the CRDT layer converges," and -/// this test reaches that layer. +/// re-run (framing Q8). +/// +/// **F2 correction.** Finding 5's first resolution ("(B): jitter-mode +/// delays both CellDelta and CrdtOp") was wrong — it widened the +/// match in the render-message loop, which never carries broadcast +/// `CrdtOp`s; the CRDT-convergence path was *not* exercised and this +/// test silently asserted nothing about CRDT-under-jitter. F2 moved +/// the jitter to the actual `broadcast_crdt_op` write site +/// (`daemon.rs`), so delivered ops are now genuinely delayed. +/// +/// **Falsification guard.** Because a no-op jitter (or jitter at the +/// wrong site, the original bug) would let convergence happen in +/// sub-millisecond time, this test asserts a **wall-clock floor**: +/// with `JITTER_MS=50` applied to every broadcast `CrdtOp` write, +/// first-send→convergence must take materially longer than +/// un-jittered delivery. A regression that detaches the jitter from +/// the CRDT path again fails the floor, not just the (weaker) +/// convergence assertion. This is the reviewer's "no-op jitter +/// cannot pass" requirement made executable. /// /// The load-bearing CRDT property: **convergence is /// delivery-order-independent.** Jitter reorders/delays op delivery; -/// the converged result must be *identical to the no-jitter result* -/// (jitter changes timing, never the CRDT outcome). The expected -/// string is pinned (record-and-assert-stable, mirroring cat-1): a -/// change signals either a loro-determinism regression or jitter -/// leaking into the CRDT outcome — both real bugs. +/// the converged result must be *identical to the no-jitter result*. +/// Pinned (record-and-assert-stable, mirroring cat-1). /// /// One scenario, not a sweep (Q8 scope guard; a fuzz sweep is v0.2). -/// Generous timeout: 50ms jitter × every CellDelta+CrdtOp write +/// Generous timeout: 50ms jitter × every broadcast-CrdtOp write /// accumulates; convergence-within-timeout, not per-event budget /// (Q5: PTY/jitter paths have no perf gate). /// @@ -1199,6 +1210,17 @@ fn m10_11_q8_convergence_under_jitter() { let replica_b = CrdtState::new(hello_b.assigned_frontend_id.0).expect("B new"); replica_b.import_snapshot(&snap_b).expect("B import"); + // Falsification-guard timer: started before the first send, + // measured after convergence. With JITTER_MS=50 applied to every + // broadcast-CrdtOp write (the F2-corrected site), first-send → + // convergence accumulates tens-to-hundreds of ms. Un-jittered + // synthetic in-process convergence is sub-10ms. A 30ms floor sits + // far above un-jittered and far below the jittered expectation — + // non-flaky in both directions, and a regression that detaches + // jitter from the CRDT path (Finding 5's original bug) drops + // convergence back under 10ms and fails the floor loudly. + let t0 = Instant::now(); + // Deterministic op sequence, interleaved between peers, several // positions. The daemon's jittered delivery reorders these on the // wire; the CRDT must converge to one delivery-order-independent @@ -1255,6 +1277,21 @@ fn m10_11_q8_convergence_under_jitter() { Duration::from_secs(20), ) .expect("Q8/criterion-3: CRDT layer must converge under jitter"); + let elapsed = t0.elapsed(); + + // Falsification guard: jitter must have actually delayed the + // broadcast-CrdtOp path. If this fails with a small `elapsed`, + // the jitter is detached from the CRDT path again (Finding 5's + // original bug / an F2 regression) — the convergence assertions + // below would still pass while testing nothing about + // CRDT-under-jitter. + assert!( + elapsed >= Duration::from_millis(30), + "criterion-3 jitter not reaching the broadcast-CrdtOp path: \ + converged in {elapsed:?} (un-jittered speed). With \ + JITTER_MS=50 on every broadcast write this must be \ + materially slower. This is the F2 regression guard." + ); // No op lost: all four tokens survive the jittered merge. for tok in ["a", "b", "A", "B"] {