neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

M10.11 fixes

This commit is contained in:
Levi Neuwirth 2026-05-15 22:04:42 -04:00
parent b6c07cb840
commit 8490e79bbb
3 changed files with 178 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

115
src/daemon.rs
View File

@ -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}");

59
src/optimistic.rs
View File

@ -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"
);
}
}

59
tests/m10_11_acceptance.rs
View File

@ -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"] {