fix(tui): #3033 audit fix — throttled AgentProgress no longer cancels redraws owed to other events in the same drain batch; restore pre-event accumulator value; extract agent_progress_redraw_permitted + unit tests
Co-Authored-By: Claude <noreply@anthropic.com> https://claude.ai/code/session_018zaP8vUfTAsrE38L6h6fw5
This commit is contained in:
@@ -1430,6 +1430,11 @@ async fn run_event_loop(
|
||||
break;
|
||||
}
|
||||
};
|
||||
// #3033: remember whether an EARLIER event in this drain batch
|
||||
// already requested a redraw. The AgentProgress throttle below
|
||||
// may opt the current event out of repainting, but it must not
|
||||
// cancel redraws owed to other events in the same batch.
|
||||
let redraw_requested_before_event = received_engine_event;
|
||||
received_engine_event = true;
|
||||
if app.suppress_stream_events_until_turn_complete {
|
||||
if matches!(event, EngineEvent::TurnStarted { .. }) {
|
||||
@@ -2326,15 +2331,14 @@ async fn run_event_loop(
|
||||
// status-animation timer (80ms cadence) provides a guaranteed
|
||||
// floor for sidebar updates. Data is still recorded immediately;
|
||||
// the sidebar picks it up on the next permitted redraw.
|
||||
let now = Instant::now();
|
||||
if let Some(last) = app.last_agent_progress_redraw {
|
||||
if now.duration_since(last) < Duration::from_millis(100) {
|
||||
received_engine_event = false;
|
||||
} else {
|
||||
app.last_agent_progress_redraw = Some(now);
|
||||
}
|
||||
} else {
|
||||
app.last_agent_progress_redraw = Some(now);
|
||||
if !agent_progress_redraw_permitted(
|
||||
&mut app.last_agent_progress_redraw,
|
||||
Instant::now(),
|
||||
) {
|
||||
// Restore the pre-event accumulator value: a
|
||||
// throttled progress event contributes no redraw of
|
||||
// its own, but earlier events' redraws survive.
|
||||
received_engine_event = redraw_requested_before_event;
|
||||
}
|
||||
}
|
||||
EngineEvent::AgentComplete { id, result } => {
|
||||
@@ -4668,6 +4672,21 @@ fn reconcile_turn_liveness(app: &mut App, now: Instant, has_running_agents: bool
|
||||
false
|
||||
}
|
||||
|
||||
/// #3033: gate progress-driven repaints to at most one per 100ms.
|
||||
///
|
||||
/// Returns whether the current `AgentProgress` event may request a redraw,
|
||||
/// updating the last-redraw timestamp when it may. Data updates are never
|
||||
/// throttled — only the repaint request is.
|
||||
fn agent_progress_redraw_permitted(last_redraw: &mut Option<Instant>, now: Instant) -> bool {
|
||||
match *last_redraw {
|
||||
Some(last) if now.duration_since(last) < Duration::from_millis(100) => false,
|
||||
_ => {
|
||||
*last_redraw = Some(now);
|
||||
true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn recover_engine_event_disconnect(app: &mut App) -> bool {
|
||||
let had_live_work = app.is_loading
|
||||
|| app.is_compacting
|
||||
|
||||
@@ -9303,3 +9303,64 @@ mod work_sidebar_projection_tests {
|
||||
assert_ne!(entry.status, "running");
|
||||
}
|
||||
}
|
||||
|
||||
// ── #3033: AgentProgress redraw throttle ───────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn agent_progress_redraw_throttle_permits_first_and_spaced_events() {
|
||||
let mut last_redraw = None;
|
||||
let t0 = Instant::now();
|
||||
|
||||
assert!(
|
||||
agent_progress_redraw_permitted(&mut last_redraw, t0),
|
||||
"first progress event always repaints"
|
||||
);
|
||||
assert!(
|
||||
!agent_progress_redraw_permitted(&mut last_redraw, t0 + Duration::from_millis(50)),
|
||||
"events inside the 100ms window are throttled"
|
||||
);
|
||||
assert!(
|
||||
!agent_progress_redraw_permitted(&mut last_redraw, t0 + Duration::from_millis(99)),
|
||||
"throttled events must not advance the window"
|
||||
);
|
||||
assert!(
|
||||
agent_progress_redraw_permitted(&mut last_redraw, t0 + Duration::from_millis(150)),
|
||||
"events past the window repaint again"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn throttled_progress_event_does_not_cancel_other_events_redraw() {
|
||||
// Repro for the #3033 audit finding: `received_engine_event` is a shared
|
||||
// accumulator for the whole drain batch. A throttled AgentProgress event
|
||||
// must restore the PRE-EVENT value instead of clearing the flag, so
|
||||
// redraws owed to other events (AgentSpawned, AgentList, cross-agent
|
||||
// AgentComplete...) survive.
|
||||
let t0 = Instant::now();
|
||||
let mut last_redraw = Some(t0);
|
||||
|
||||
// Batch: AgentSpawned (requests redraw), then a throttled AgentProgress.
|
||||
let mut received_engine_event = true; // AgentSpawned drained
|
||||
let redraw_requested_before_event = received_engine_event;
|
||||
received_engine_event = true; // AgentProgress drained
|
||||
if !agent_progress_redraw_permitted(&mut last_redraw, t0 + Duration::from_millis(10)) {
|
||||
received_engine_event = redraw_requested_before_event;
|
||||
}
|
||||
assert!(
|
||||
received_engine_event,
|
||||
"redraw owed to AgentSpawned must survive a throttled progress event"
|
||||
);
|
||||
|
||||
// Same batch shape but with NO earlier redraw-worthy event: the lone
|
||||
// throttled progress event contributes nothing.
|
||||
let mut received_engine_event = false;
|
||||
let redraw_requested_before_event = received_engine_event;
|
||||
received_engine_event = true; // AgentProgress drained
|
||||
if !agent_progress_redraw_permitted(&mut last_redraw, t0 + Duration::from_millis(20)) {
|
||||
received_engine_event = redraw_requested_before_event;
|
||||
}
|
||||
assert!(
|
||||
!received_engine_event,
|
||||
"a lone throttled progress event must not trigger a repaint"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user