Skip to content

Add retry with backoff for initial snapshot fetch failure #708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iamnbutler wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add retry with backoff for initial snapshot fetch failure #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions crates/desktop/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ const MAX_EVENTS: usize = 200;
/// Polling interval for snapshot refresh (milliseconds).
const POLL_INTERVAL_MS: u64 = 5_000;

/// Initial retry delay for snapshot fetch (milliseconds).
const INITIAL_RETRY_DELAY_MS: u64 = 1_000;

/// Maximum retry delay for snapshot fetch (milliseconds).
const MAX_RETRY_DELAY_MS: u64 = 30_000;

/// Check whether an event type string should trigger a snapshot refresh.
/// Matches: task:*, merge:*, system:mode*
fn is_state_changing_event(event_type: &str) -> bool {
Expand Down Expand Up @@ -118,6 +124,8 @@ pub struct AppState {
stop_polling: Option<Arc<AtomicBool>>,
/// Whether a fetch is currently in flight.
fetch_in_flight: Arc<AtomicBool>,
/// Number of consecutive snapshot fetch failures (for backoff).
snapshot_retry_count: u32,
}

impl AppState {
Expand Down Expand Up @@ -158,6 +166,7 @@ impl AppState {
sse_client: Some(sse_client),
stop_polling: None,
fetch_in_flight: Arc::new(AtomicBool::new(false)),
snapshot_retry_count: 0,
};

// Start polling loop
Expand Down Expand Up @@ -325,8 +334,24 @@ impl AppState {
}
}

/// Get the current snapshot retry count.
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION]

Priority: Code Quality

snapshot_retry_count() is a getter but it's placed in the // --- Setters --- section (between set_selected_project and // --- Actions ---). Move it up to join the other getters under // --- Getters ---.

pub fn snapshot_retry_count(&self) -> u32 {
self.snapshot_retry_count
}

// --- Actions ---

/// Manually retry connecting to the server (resets backoff).
pub fn retry_connection(&mut self, cx: &mut Context<Self>) {
self.snapshot_retry_count = 0;
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION]

Priority: Correctness

retry_connection() resets snapshot_retry_count to 0 and sets status to Connecting, but any previously-detached retry timers remain active. If a stale timer fires after this reset (while the manually-triggered fetch is still in-flight or has already completed), update_snapshot on that path will flip the status back to Reconnecting — causing a visible flicker from Connecting → Reconnecting shortly after the user clicks Retry.

Since GPUI tasks are spawned with .detach(), there's no cancellation handle. A simple mitigation is a generation counter:

retry_generation: u32,  // bumped in retry_connection()

Pass the current generation into the timer closure, and in the closure, bail out if the generation has changed:

cx.spawn(async move |this, cx| {
    smol::Timer::after(delay).await;
    this.update(cx, |state, cx| {
        if state.retry_generation == generation {
            state.refresh_snapshot(cx);
        }
    }).ok();
}).detach();

self.connection_status = ConnectionStatus::Connecting;
cx.emit(AppStateEvent::ConnectionStatusChanged(
self.connection_status,
));
cx.notify();
self.refresh_snapshot(cx);
}

/// Refresh the snapshot from the server.
pub fn refresh_snapshot(&mut self, cx: &mut Context<Self>) {
// Prevent concurrent fetches
Expand Down Expand Up @@ -360,9 +385,12 @@ impl AppState {
Ok(snapshot) => {
self.snapshot = Some(snapshot);
self.last_error = None;
self.snapshot_retry_count = 0;
// Update connection status if we were previously disconnected
if self.connection_status == ConnectionStatus::Disconnected
|| self.connection_status == ConnectionStatus::Failed
|| self.connection_status == ConnectionStatus::Connecting
|| self.connection_status == ConnectionStatus::Reconnecting
{
self.connection_status = ConnectionStatus::Connected;
cx.emit(AppStateEvent::ConnectionStatusChanged(
Expand All @@ -373,7 +401,41 @@ impl AppState {
cx.notify();
}
Err(e) => {
let had_snapshot = self.snapshot.is_some();
self.set_error(e.to_string(), cx);

// If we've never had a snapshot, schedule a retry with backoff
if !had_snapshot {
self.snapshot_retry_count += 1;
let delay_ms = INITIAL_RETRY_DELAY_MS
.saturating_mul(1 << self.snapshot_retry_count.min(5))
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION]

Priority: Code Quality

The backoff timers are redundant with the 5s polling loop during the pre-connection phase. When both are active, each polling tick also calls update_snapshot and, on failure, spawns its own new retry timer. Over time this accumulates multiple concurrent timers all scheduled at the max 30s interval — each one that fires (and fails) schedules yet another 30s timer. The fetch_in_flight guard prevents duplicate HTTP requests, but the effective retry rate in steady state is dominated by the 5s polling, not the 30s backoff cap.

The retry timers do add value for the very first window (2s faster than polling's 5s), but the rate-limiting intent of the backoff doesn't hold once the polling loop joins in.

One cleaner approach: only schedule a retry from update_snapshot when snapshot_retry_count == 1 (i.e., skip the retry timer on polling failures, only use it for the very first failure). Or track a flag like retry_timer_pending to skip scheduling a new one when one is already active.

.min(MAX_RETRY_DELAY_MS);

// Show reconnecting status during retries
if self.connection_status != ConnectionStatus::Reconnecting {
self.connection_status = ConnectionStatus::Reconnecting;
cx.emit(AppStateEvent::ConnectionStatusChanged(
self.connection_status,
));
cx.notify();
}

info!(
retry_count = self.snapshot_retry_count,
delay_ms, "Initial snapshot fetch failed, retrying"
);

let delay = Duration::from_millis(delay_ms);
cx.spawn(async move |this, cx| {
smol::Timer::after(delay).await;
if let Err(e) = this.update(cx, |state, cx| {
state.refresh_snapshot(cx);
}) {
warn!(error = %e, "Failed to trigger snapshot retry");
}
})
.detach();
}
}
}
}
Expand Down
58 changes: 49 additions & 9 deletions crates/desktop/src/views/dashboard.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use chrono::{DateTime, Utc};
use gpui::{div, prelude::*, Entity, FontWeight, Styled, Window};

use crate::api::{self, TaskState};
use crate::components::{Badge, BadgeVariant, Card, CardContent, CardHeader};
use crate::state::AppState;
use crate::components::{Badge, BadgeVariant, Button, ButtonVariant, Card, CardContent, CardHeader};
use crate::state::{AppState, ConnectionStatus};
use crate::theme::{colors, radius, rgb, spacing, style_helpers::StyledExt, typography, ComponentTheme};

/// Maximum number of recent events to display.
Expand All @@ -37,18 +37,58 @@ impl Render for Dashboard {

// Check if we have data
let Some(snapshot) = state.snapshot() else {
return div()
let connection_status = state.connection_status();
let last_error = state.last_error().map(|s| s.to_string());
let state_entity = self.state.clone();

let (status_text, show_retry) = match connection_status {
ConnectionStatus::Connecting => ("Connecting to server...", false),
ConnectionStatus::Reconnecting => ("Reconnecting to server...", true),
ConnectionStatus::Failed => ("Failed to connect to server", true),
ConnectionStatus::Disconnected => ("Disconnected from server", true),
ConnectionStatus::Connected => ("Waiting for data...", false),
};

let mut container = div()
.size_full()
.flex()
.flex_col()
.items_center()
.justify_center()
.child(
.gap(spacing::SPACE_3);

container = container.child(
div()
.text_muted()
.text_size(typography::TEXT_SM)
.child(status_text.to_string()),
);

if let Some(err) = last_error {
container = container.child(
div()
.text_muted()
.text_size(typography::TEXT_SM)
.child("No data yet"),
)
.into_any_element();
.text_size(typography::TEXT_XS)
.text_color(rgb(colors::DESTRUCTIVE))
.max_w(gpui::px(400.0))
.text_ellipsis()
.overflow_hidden()
.child(err),
);
}

if show_retry {
container = container.child(
Button::new("retry-connection", "Retry")
.variant(ButtonVariant::Outline)
.on_click(move |_event, _window, cx| {
state_entity.update(cx, |state, cx| {
state.retry_connection(cx);
});
}),
);
}

return container.into_any_element();
};

// Calculate stats
Expand Down
Loading