From 7dc6817238429728a2f96c0d9c8390dc147b02cb Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Mon, 21 Sep 2026 12:38:29 -0300 Subject: [PATCH] test(drm): drive a legacy cursor message from the wire, not from a literal The upgrade window is the one thing in this round that could not be staged with two live processes: a --server from one build does not run inside the other's tree, so the old service respawns it and it dies with exit 127. Tried on two boxes, with two files swapped and with the whole tree. This is the honest form of that test and it runs in CI rather than once on a desk. It takes the exact bytes an old producer puts on the socket -- no hot_measured key at all, adjacently tagged as Data really is -- deserializes them the way the receive loop does, drives the real delivery path, and reads back what was published. The transform is non-zero on purpose: at 0 the path never consults hot_measured, so a test there could not tell the branches apart, and the fixture is asserted to make mapping and re-inferring disagree before anything is concluded from them. Verified by mutation: restoring #[serde(default)] -- the reported bug -- fails it, and so does a delivery path that ignores hot_measured. --- src/server/drm_capturer.rs | 114 +++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index e85d25160..aa9b44d03 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -966,6 +966,7 @@ fn deliver_drm_cursor( } else { (width as i32, height as i32, hotx, hoty, raw) }; + note_received_provenance(hot_measured); let id = fold_cursor_id(id, t); set_drm_cursor( display, @@ -981,6 +982,34 @@ fn deliver_drm_cursor( ); } +static LAST_RECEIVED_PROVENANCE: std::sync::atomic::AtomicI8 = std::sync::atomic::AtomicI8::new(-1); + +/// Say once, and again only on a change, what the producer told us about the hotspot. +/// +/// The counterpart of the producer's own provenance line, and it exists for the case that line +/// cannot cover: an UPGRADE. The two processes are replaced separately, so an old service can be +/// streaming to a freshly started `--server`, and its `DrmCursor` carries no `hot_measured` at all. +/// What this consumer then does with it is decided by the field's serde default, and without this +/// line that decision is invisible on a running box -- which is exactly the window where it +/// matters and the hardest one to reproduce on purpose. +fn note_received_provenance(hot_measured: bool) { + let now = hot_measured as i8; + if LAST_RECEIVED_PROVENANCE.swap(now, std::sync::atomic::Ordering::Relaxed) == now { + return; + } + if hot_measured { + log::info!( + "drm: cursor hotspots arrive MEASURED; mapping the point the producer sent. \ + (a producer that predates the field reads as measured too, on purpose: that \ + preserves what the old protocol meant)" + ); + } else { + log::info!( + "drm: cursor hotspots arrive as a GUESS; re-inferring from the upright bitmap" + ); + } +} + fn fold_cursor_id(id: u64, t: i32) -> u64 { if id == scrap::drm_reader::HIDDEN_CURSOR_ID { id @@ -2082,6 +2111,91 @@ mod drm_capturer_tests { // whichever way it lies, and that is what this checks - for both orientations and at the real // 2.22:1 aspect of the shape that was wrong - by driving `deliver_drm_cursor` and reading the // published hotspot back, not by chaining the helpers itself. + /// The upgrade window, end to end from the WIRE rather than from a struct literal. + /// + /// An old root service can be streaming to a freshly started `--server`, and its `DrmCursor` + /// carries no `hot_measured` at all. This takes the exact bytes such a producer puts on the + /// socket, deserializes them the way the consumer's receive loop does, and drives the real + /// delivery path, then reads back what was published. Staging it with two live processes is + /// not possible with how the package is built - a `--server` from one build will not run + /// inside the other's tree - so this is the honest form of that test, and it runs in CI + /// instead of once on somebody's desk. + /// + /// The transform must be non-zero: at 0 the delivery path never consults `hot_measured`, so a + /// test at 0 could not tell the two branches apart. + #[test] + fn a_legacy_cursor_message_keeps_the_hotspot_the_old_protocol_meant() { + use crate::ipc::Data; + + let (up, w, h, _box_centre) = ibeam_theme_aspect(); + let t = 90; + let (scan, sw, sh) = as_scanned_out(&up, w, h, t); + // A hotspot the old producer measured and sent as a point on the scanned-out sprite. + let (hotx, hoty) = (3, 5); + + // Exactly what an old producer writes: `Data` is adjacently tagged, and there is no + // `hot_measured` key because that build has no such field. + let legacy = format!( + r#"{{"t":"DrmCursor","c":{{"id":11,"width":{sw},"height":{sh},"hotx":{hotx},"hoty":{hoty}}}}}"# + ); + let (id, mw, mh, mx, my, measured) = + match serde_json::from_str::(&legacy).expect("a legacy DrmCursor must parse") { + Data::DrmCursor { id, width, height, hotx, hoty, hot_measured } => { + (id, width, height, hotx, hoty, hot_measured) + } + other => panic!("expected DrmCursor, got {other:?}"), + }; + assert!(measured, "no provenance on the wire must mean what the old protocol meant"); + + // The two branches must disagree here, or the assertion below proves nothing. + let mapped = unrotate_hotspot(t, mw as i32, mh as i32, mx, my); + let mut turned = Vec::new(); + unrotate_bgra(&scan, sw, sh, t, &mut turned); + let (dw, dh) = rotated_dims(t, sw, sh); + let reinferred = scrap::drm_reader::infer_hotspot(&turned, dw, dh); + assert_ne!( + mapped, reinferred, + "fixture cannot discriminate: mapping and re-inferring give the same point" + ); + + let display = 9_400; + deliver_drm_cursor(display, 1, id, mw, mh, mx, my, measured, scan, t); + let published = { + let map = DRM_CURSOR.lock().unwrap(); + let (_, c) = map.get(&display).expect("the delivery path published nothing"); + (c.hotx, c.hoty) + }; + assert_eq!( + published, mapped, + "a legacy message must keep the hotspot the producer sent, transformed as a point; \ + re-inferring it would move the cursor for the length of an upgrade" + ); + + // The control: a NEW producer asking for re-inference says so explicitly, and that must + // not be swallowed by the default. + let modern = format!( + r#"{{"t":"DrmCursor","c":{{"id":12,"width":{sw},"height":{sh},"hotx":{hotx},"hoty":{hoty},"hot_measured":false}}}}"# + ); + let Data::DrmCursor { hot_measured, .. } = + serde_json::from_str::(&modern).expect("a modern DrmCursor must parse") + else { + panic!("expected DrmCursor") + }; + assert!(!hot_measured); + let (scan2, _, _) = as_scanned_out(&up, w, h, t); + deliver_drm_cursor(display + 1, 1, 12, sw as u32, sh as u32, hotx, hoty, false, scan2, t); + let published2 = { + let map = DRM_CURSOR.lock().unwrap(); + let (_, c) = map.get(&(display + 1)).expect("nothing published for the control"); + (c.hotx, c.hoty) + }; + assert_eq!( + published2, reinferred, + "an explicit false must still re-infer, or the legacy default has eaten the new \ + producer's request" + ); + } + #[test] fn an_ibeam_keeps_its_centre_whichever_way_it_lies() { use scrap::drm_reader::infer_hotspot;