fix(agent-core): retry volatile DPI re-apply on cold boot#449
fix(agent-core): retry volatile DPI re-apply on cold boot#449iamshakibali wants to merge 1 commit into
Conversation
The MX Master 3s is slow to enumerate after a power cycle, so the single confirming re-apply could time out against a still-booting device and DPI would revert to the hardware default. Retry for a bounded run of ticks instead of one-shot.
Greptile SummaryThis PR changes the volatile-settings re-apply for newly detected devices from a one-shot confirming write to a bounded-retry loop (
Confidence Score: 4/5Safe to merge — the change is confined to a single file and adds retries without removing any existing re-apply triggers. The cold-boot retry logic is sound and well-tested for the primary path. Two edge cases are worth a follow-up: the retry budget is silently dropped when a system wake fires while a device is mid-retry, and the first-sighting filter and the followup map key on different identifiers (stable_id vs config_key), which could produce unexpected behavior on a rapid replug. Neither is a regression from the previous code, and both are unlikely in normal usage. The plan_reapply function in orchestrator.rs — specifically the interaction between reapply_all and an active followup retry budget, and the mixed use of stable_id vs config_key as map keys. Important Files Changed
Sequence DiagramsequenceDiagram
participant HW as Device (cold boot)
participant INV as Inventory Watcher
participant ORCH as Orchestrator / plan_reapply
participant FU as reapply_followup
Note over HW: Device powers on (slow to enumerate)
INV->>ORCH: refresh_inventory (tick 0)
ORCH->>ORCH: "reapply_targets first sighting targets=[dev]"
ORCH->>HW: reapply_volatile_settings (may time out, device still booting)
ORCH->>FU: "followup[dev] = VOLATILE_REAPPLY_CONFIRM_RETRIES (4)"
INV->>ORCH: refresh_inventory (tick 1)
ORCH->>ORCH: "reapply_targets steady state targets=[]"
ORCH->>ORCH: "followup[dev]=4 push dev set next[dev]=3"
ORCH->>HW: reapply_volatile_settings (device likely still booting)
ORCH->>FU: "followup[dev] = 3"
INV->>ORCH: refresh_inventory (tick 2)
Note over ORCH: same logic, remaining=3 to 2
ORCH->>HW: reapply_volatile_settings
INV->>ORCH: refresh_inventory (tick 3)
Note over ORCH: remaining=2 to 1
ORCH->>HW: reapply_volatile_settings
INV->>ORCH: refresh_inventory (tick 4 final retry)
ORCH->>ORCH: "followup[dev]=1 push dev remaining le 1 no next entry"
ORCH->>HW: reapply_volatile_settings (device fully booted, write lands)
ORCH->>FU: "followup = {} (budget exhausted)"
INV->>ORCH: refresh_inventory (tick 5+)
ORCH->>ORCH: "no followup entry targets=[] steady state"
Reviews (1): Last reviewed commit: "fix(agent-core): retry volatile DPI re-a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (idx, dev) in next.iter().enumerate() { | ||
| if dev.online | ||
| && dev.route.is_some() | ||
| && followup.contains(&dev.config_key) | ||
| && !targets.contains(&idx) | ||
| { | ||
| targets.push(idx); | ||
| if dev.online && dev.route.is_some() && !targets.contains(&idx) { | ||
| // ponytail: bounded retry, not read-back-confirmed. The upgrade is | ||
| // to confirm the write took (read DPI back via openlogi_hid and | ||
| // stop retrying on a match) instead of running out a fixed budget — | ||
| // that converges faster and drops the redundant writes on a device | ||
| // that accepted the first one. | ||
| if let Some(&remaining) = followup.get(&dev.config_key) { | ||
| targets.push(idx); | ||
| if remaining > 1 { | ||
| next_followup.insert(dev.config_key.clone(), remaining - 1); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When a system wake fires (reapply_all = true) while a device is mid-retry (e.g., followup["dev"] = 2), the device lands in targets via reapply_targets (all online devices are included). Because !targets.contains(&idx) is then false, the followup retry loop never fires, and the existing budget entry is not copied into next_followup — it is silently discarded.
The device gets one reapply_all write, but if that write also times out against a still-booting device, no further retries follow. This narrows the fix's window of effect: a system wake arriving early in the cold-boot retry sequence leaves the device with the same single-shot behaviour that this PR is trying to fix.
Consider preserving the remaining budget when reapply_all targets a device that already has a followup entry, for example by carrying forward min(remaining, VOLATILE_REAPPLY_CONFIRM_RETRIES) into next_followup even when the device is already in targets.
| let mut next_followup: HashMap<String, u8> = targets | ||
| .iter() | ||
| .filter(|&&idx| { | ||
| let id = stable_id(&next[idx]); | ||
| !prev.iter().any(|p| stable_id(p) == id) | ||
| }) | ||
| .map(|&idx| next[idx].config_key.clone()) | ||
| .map(|&idx| { | ||
| ( | ||
| next[idx].config_key.clone(), | ||
| VOLATILE_REAPPLY_CONFIRM_RETRIES, | ||
| ) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
next_followup construction may clobber an in-flight retry entry on rapid replug
next_followup is first populated from the first-sighting targets using .collect() on the iterator. If config_key is derived solely from the device's serial/unit_id (i.e., the same key is preserved across a replug to a new Bolt slot), a device that was mid-retry and also appears as a "first sighting" this tick (because its stable_id changed with the new slot) has its existing budget reset to VOLATILE_REAPPLY_CONFIRM_RETRIES rather than inheriting the remaining count from followup. The retry window is restarted in full.
This is harmless in practice (a replug resets enumeration anyway), but worth confirming that config_key round-trips through a slot change as expected, since the followup map and the first-sighting filter key on different things (config_key vs stable_id).
1 participant
What
Change the volatile-settings re-apply for first-sighted devices from one-shot to bounded-retry.
Why
The MX Master 3s is slow to enumerate after a reboot. The single confirming re-apply could time out against a still-booting mouse, so DPI (which lives in device RAM and clears on power cycle) reverted to the hardware default and stayed there until the next sleep/wake.
How
reapply_followupchanges fromHashSet<String>(one confirm) toHashMap<String, u8>(retry budget per device key).VOLATILE_REAPPLY_CONFIRM_RETRIES = 4(~8s at the 2s inventory cadence).Files
crates/openlogi-agent-core/src/orchestrator.rsTest
plan_reapply_retries_a_first_sighting_for_a_bounded_runcovers the decrement-and-exhaust path.-D warnings+ fmt clean.