Add exponential backoff to Fleet Desktop server communications - #45623
Merged
Conversation
Fleet Desktop now backs off exponentially (with jitter) when receiving errors from the Fleet server, preventing request storms that can overwhelm the database. This addresses corrective action #3 from the #44816 postmortem, where expired-token polling without backoff caused a DB outage. Changes: - New `orbit/pkg/backoff` package: stateful tracker with exponential doubling, jitter, max cap, and per-path isolation - Integrated into Desktop's ping loop and checkToken retry loop - 13 unit tests covering all Oracle scenarios from #45553 Closes #45553
This was referenced May 15, 2026
Three new tests that verify backoff behavior with actual time.Ticker instances and wall-clock measurements, matching how Desktop uses the tracker in its polling loop: - TestTickerIntegration: 4 consecutive failures with measured growing intervals, then success resets to base - TestTickerIntegrationMaxCap: verifies ticker caps at maxBackoff - TestMultipleTrackersWithTickers: per-path isolation with real tickers
Three tests that verify backoff against real HTTP servers with togglable error responses, connection-refused scenarios, and measured wall-clock timing: - TestManualBackoffAgainstHTTPServer: full lifecycle (healthy -> 401 errors -> recovery -> 500 errors -> recovery) with real HTTP round-trips - TestManualBackoffServerDown: connection-refused backoff - TestManualBackoffMaxCapWithRealServer: continuous 401s until cap Also manually tested against a live Fleet server: - Invalid token -> 401 backoff: 500ms -> 1s -> 2.2s -> 4s -> 8s -> 10s (cap) - Server killed mid-poll -> seamless transition to network-error backoff - Recovery -> instant reset to base interval
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45623 +/- ##
==========================================
+ Coverage 66.73% 66.81% +0.07%
==========================================
Files 2732 2761 +29
Lines 218551 222807 +4256
Branches 10840 10840
==========================================
+ Hits 145857 148872 +3015
- Misses 59479 60320 +841
- Partials 13215 13615 +400
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- gosec: suppress G404 on jitter rand (not security-sensitive) - gocritic: use fleethttp.NewClient instead of raw http.Client in tests - revive: remove redundant type from var declaration - Add changes/45624-desktop-exponential-backoff
New() now enforces baseInterval >= 1s and maxBackoff >= baseInterval, so Interval() can never return a zero or negative duration that would panic a ticker. Added test for garbage inputs (0, negative, inverted).
sharon-fdm
commented
May 25, 2026
| interval = t.maxBackoff | ||
| } | ||
| interval += jitter(interval) | ||
| interval = min(interval, t.maxBackoff) |
Collaborator
Author
There was a problem hiding this comment.
For the reviewer:
This code section (lines 94-102) is critical so I did not trust AI.
Please understand it thoroughly and approve or propose changes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable exponential backoff mechanism for agent-to-server communication and applies it to Fleet Desktop polling paths to prevent error-driven request storms (notably the expired-token scenario behind #44816 / #45624).
Changes:
- Added a new
orbit/pkg/backoffpackage implementing a thread-safe, stateful exponential backoff tracker with jitter and per-path isolation. - Integrated backoff into Fleet Desktop’s ping loop and the
checkTokenretry loop. - Added unit + timing/integration-style tests for backoff behavior and ticker-loop integration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| orbit/pkg/backoff/backoff.go | Implements the backoff tracker (interval calculation, jitter, state tracking). |
| orbit/pkg/backoff/backoff_test.go | Adds unit tests and ticker-integration timing tests for the tracker. |
| orbit/pkg/backoff/manual_test.go | Adds HTTP/connection-refused integration tests for real round-trips/backoff behavior. |
| orbit/cmd/desktop/desktop.go | Applies backoff to Fleet Desktop ping and token-check retry loops. |
| orbit/changes/45624-desktop-exponential-backoff | Adds changelog entry describing the new behavior. |
Comments suppressed due to low confidence (1)
orbit/cmd/desktop/desktop.go:367
- This comment is now inaccurate: with exponential backoff enabled, the 6 retries used for the offline indicator can take substantially longer than
~1m (6 * 10s). Update the comment to reflect the new behavior or remove the fixed-duration estimate.
// We try 5 more times to make sure one bad request doesn't trigger the offline indicator.
// So it might take up to ~1m (6 * 10s) for Fleet Desktop to show the offline indicator.
if pingErrCount >= 6 {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+56
| // New creates a Tracker with the given base polling interval and maximum | ||
| // backoff ceiling. Both values are floored at minInterval (1s) to | ||
| // guarantee Interval never returns a value that would panic a ticker. | ||
| func New(baseInterval, maxBackoff time.Duration) *Tracker { | ||
| if baseInterval < minInterval { | ||
| baseInterval = minInterval | ||
| } | ||
| if maxBackoff < minInterval { | ||
| maxBackoff = minInterval | ||
| } | ||
| return &Tracker{ | ||
| baseInterval: baseInterval, | ||
| maxBackoff: maxBackoff, | ||
| } |
Comment on lines
+25
to
+29
| var serverStatus = http.StatusOK | ||
| srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(serverStatus) | ||
| fmt.Fprintf(w, `{"status":%d}`, serverStatus) | ||
| })) |
Comment on lines
+351
to
+363
| // Reset the ticker to the appropriate interval. Normally this | ||
| // is pingInterval, but during backoff it increases exponentially. | ||
| // The 1ms reset from user clicks will be overridden here. | ||
| pingTicker.Reset(pingBackoff.Interval()) | ||
|
|
||
| if err := client.Ping(); err != nil { | ||
| log.Error().Err(err).Int("count", pingErrCount).Msg("ping failed") | ||
| pingBackoff.RecordFailure() | ||
| // Reset the ticker to the new (longer) backoff interval. | ||
| pingTicker.Reset(pingBackoff.Interval()) | ||
|
|
||
| log.Error().Err(err).Int("count", pingErrCount). | ||
| Str("next_retry", pingBackoff.Interval().String()). | ||
| Msg("ping failed, backing off") |
Comment on lines
351
to
355
| // Reset the ticker to the appropriate interval. Normally this | ||
| // is pingInterval, but during backoff it increases exponentially. | ||
| // The 1ms reset from user clicks will be overridden here. | ||
| pingTicker.Reset(pingBackoff.Interval()) | ||
|
|
Contributor
WalkthroughThis PR introduces exponential backoff with jitter to Fleet Desktop's server polling to reduce load during transient failures. It adds a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
orbit/pkg/backoff/backoff_test.go (1)
20-34: ⚡ Quick winAdd a regression test for
baseInterval > maxBackoffinputs.There’s no coverage for the constructor edge case where callers pass a lower
maxBackoffthanbaseInterval; adding it will lock in the expected normalization and prevent inverted backoff regressions.Proposed test
func TestNewFloorsGarbageInputs(t *testing.T) { @@ } + +func TestNewNormalizesMaxBackoffToBaseInterval(t *testing.T) { + tr := New(10*time.Second, 5*time.Second) + assert.Equal(t, 10*time.Second, tr.baseInterval) + assert.Equal(t, 10*time.Second, tr.maxBackoff) + + tr.RecordFailure() + assert.GreaterOrEqual(t, tr.Interval(), 10*time.Second) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orbit/pkg/backoff/backoff_test.go` around lines 20 - 34, Add a regression case to TestNewFloorsGarbageInputs that constructs a backoff with baseInterval greater than maxBackoff (call New with base > max), then assert that the created tr.normalizes values so tr.baseInterval >= minInterval and tr.maxBackoff >= tr.baseInterval; after calling tr.RecordFailure() assert tr.Interval() >= minInterval as well. Use the existing symbols New, TestNewFloorsGarbageInputs, tr.baseInterval, tr.maxBackoff, tr.RecordFailure, tr.Interval and minInterval so the test covers the constructor normalization when baseInterval > maxBackoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@orbit/pkg/backoff/backoff.go`:
- Around line 46-56: The constructor New currently allows baseInterval >
maxBackoff which can invert backoff; in New (and the other constructor variant
around the 90-103 block) ensure after normalizing both baseInterval and
maxBackoff against minInterval you also enforce maxBackoff >= baseInterval
(e.g., if maxBackoff < baseInterval set maxBackoff = baseInterval) so failures
never produce a shorter wait; update the logic in the New function and the
duplicate constructor to perform this check on the Tracker fields baseInterval
and maxBackoff.
In `@orbit/pkg/backoff/manual_test.go`:
- Around line 146-159: The test uses a fixed port
("https://127.0.0.1:19999/healthz") which can be occupied on CI; instead obtain
an ephemeral TCP port, close the listener to ensure nothing is listening, and
use that port in the URL so connection attempts fail reliably. In manual_test.go
modify the outage simulation: call net.Listen("tcp","127.0.0.1:0"), read the
actual port from ln.Addr(), close ln, then pass the constructed
"https://127.0.0.1:<port>/healthz" to fleethttp.NewClient/client.Get so the loop
that calls client.Get will hit a free-but-closed port and trigger the expected
connection-refused behavior.
---
Nitpick comments:
In `@orbit/pkg/backoff/backoff_test.go`:
- Around line 20-34: Add a regression case to TestNewFloorsGarbageInputs that
constructs a backoff with baseInterval greater than maxBackoff (call New with
base > max), then assert that the created tr.normalizes values so
tr.baseInterval >= minInterval and tr.maxBackoff >= tr.baseInterval; after
calling tr.RecordFailure() assert tr.Interval() >= minInterval as well. Use the
existing symbols New, TestNewFloorsGarbageInputs, tr.baseInterval,
tr.maxBackoff, tr.RecordFailure, tr.Interval and minInterval so the test covers
the constructor normalization when baseInterval > maxBackoff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37486584-5999-4180-9445-8402d65b3284
📒 Files selected for processing (5)
orbit/changes/45624-desktop-exponential-backofforbit/cmd/desktop/desktop.goorbit/pkg/backoff/backoff.goorbit/pkg/backoff/backoff_test.goorbit/pkg/backoff/manual_test.go
Comment on lines
+46
to
+56
| func New(baseInterval, maxBackoff time.Duration) *Tracker { | ||
| if baseInterval < minInterval { | ||
| baseInterval = minInterval | ||
| } | ||
| if maxBackoff < minInterval { | ||
| maxBackoff = minInterval | ||
| } | ||
| return &Tracker{ | ||
| baseInterval: baseInterval, | ||
| maxBackoff: maxBackoff, | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce maxBackoff >= baseInterval in New to prevent inverted backoff.
When baseInterval is greater than maxBackoff, a failure can shorten the wait time instead of increasing it, which defeats outage-throttling behavior.
Proposed fix
func New(baseInterval, maxBackoff time.Duration) *Tracker {
if baseInterval < minInterval {
baseInterval = minInterval
}
if maxBackoff < minInterval {
maxBackoff = minInterval
}
+ if maxBackoff < baseInterval {
+ maxBackoff = baseInterval
+ }
return &Tracker{
baseInterval: baseInterval,
maxBackoff: maxBackoff,
}
}Also applies to: 90-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orbit/pkg/backoff/backoff.go` around lines 46 - 56, The constructor New
currently allows baseInterval > maxBackoff which can invert backoff; in New (and
the other constructor variant around the 90-103 block) ensure after normalizing
both baseInterval and maxBackoff against minInterval you also enforce maxBackoff
>= baseInterval (e.g., if maxBackoff < baseInterval set maxBackoff =
baseInterval) so failures never produce a shorter wait; update the logic in the
New function and the duplicate constructor to perform this check on the Tracker
fields baseInterval and maxBackoff.
lucasmrod
reviewed
Jun 2, 2026
- Move backoff from Ping (unauthenticated, no DB) to DesktopSummary (the actual DB-hitting call that caused #44816) - Separate maxTokenBackoff (5min) for checkToken path - Reduce max backoff from 30min to 5min (still 60x reduction, better UX on recovery) - Reduce checkToken base from 5s to 1s (backoff makes aggressive start safe) - Add t.Parallel() to all tests - Add -race comment to TestConcurrentAccess - Remove manual_test.go (redundant with backoff_test.go)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@orbit/cmd/desktop/desktop.go`:
- Around line 355-356: The code stamps lastDesktopSummaryCheck before making the
DesktopSummary request which causes the desktopSummaryInterval guard to suppress
retries even when summaryBackoff wants faster retries; move the update so the
timestamp is only set on success-like paths (after a successful DesktopSummary
or any path that should count as a resolved attempt) and do not update
lastDesktopSummaryCheck when calling summaryBackoff.RecordFailure() and
pingTicker.Reset(...); apply the same change to the other DesktopSummary branch
(the block containing the similar update around
summaryBackoff.RecordFailure()/pingTicker.Reset) so backoff controls retry
cadence instead of the five-minute guard.
- Around line 391-392: The ErrMissingLicense branch treats the client as
connected but skips rescheduling the pingTicker, so recovery waits on the old
backoff interval; after calling summaryBackoff.RecordSuccess() and
menuManager.SetConnected(...), stop and recreate or otherwise reset pingTicker
to the free-tier/default interval (e.g., call pingTicker.Stop() and assign a new
ticker at the normal interval or invoke the existing ticker-reset helper) before
executing the continue so the ping loop immediately moves to the free-tier
schedule; ensure you reference ErrMissingLicense,
summaryBackoff.RecordSuccess(), menuManager.SetConnected, and pingTicker when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 639839f7-ecf0-443d-a1e6-bb0e7a980bcc
📒 Files selected for processing (2)
orbit/cmd/desktop/desktop.goorbit/pkg/backoff/backoff_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- orbit/pkg/backoff/backoff_test.go
lucasmrod
reviewed
Jun 2, 2026
lucasmrod
reviewed
Jun 2, 2026
lucasmrod
left a comment
lucasmrod
left a comment
Member
There was a problem hiding this comment.
Left a comment to remove more code which seems not necessary. So we should only keep the 5s token check -> (1s, max 5m) change.
Per Lucas's review: DesktopSummary in the main loop already runs at most every 5 minutes (desktopSummaryInterval guard). The tight retry loop that caused #44816 is in checkToken, which already has backoff. Removed summaryBackoff tracker from the main loop entirely.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@orbit/cmd/desktop/desktop.go`:
- Around line 392-395: The auth retry path is starting its own checkToken()
backoff while the token watcher can also start checkToken(), causing duplicate
backoff loops; change the logic so only one in-flight checkToken() is ever
started and all callers wait on its returned done channel. Implement a shared
in-flight indicator (e.g., a package-scoped variable or small single-flight
wrapper) that holds the current done channel returned by checkToken(); on
DesktopSummary auth failure and in the token watcher, check that indicator and
either reuse/wait on the existing done channel or atomically set and start a new
checkToken() and store its done channel until it completes, then clear the
indicator. Ensure synchronization (mutex/atomic) around the shared indicator so
concurrent callers race to reuse the same in-flight checkToken() rather than
spawning duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b33c3047-a8cc-481b-ace2-102b5cd6ae8b
📒 Files selected for processing (1)
orbit/cmd/desktop/desktop.go
Comment on lines
392
to
395
| log.Debug().Err(err).Msg("get desktop summary auth failure") | ||
| // This usually happens every ~1 hour when the token expires. | ||
| // checkToken has its own backoff to avoid retry storms (#44816). | ||
| <-checkToken() |
Contributor
There was a problem hiding this comment.
Deduplicate checkToken() so the auth path only has one backoff loop.
This branch now assumes checkToken() is the single retry throttle, but checkToken() can also be started from the token watcher on Lines 331-332. If those overlap, you end up with multiple goroutines issuing DesktopSummary retries with separate tokenBackoff trackers, which weakens the outage protection this PR is adding. Keep one in-flight checkToken() loop and have later callers wait on the same done channel instead of starting another one.
Possible direction
+ var checkTokenMu sync.Mutex
+ var checkTokenInFlight <-chan interface{}
+
checkToken := func() <-chan interface{} {
+ checkTokenMu.Lock()
+ if checkTokenInFlight != nil {
+ done := checkTokenInFlight
+ checkTokenMu.Unlock()
+ return done
+ }
+
menuManager.SetConnecting()
done := make(chan interface{})
+ checkTokenInFlight = done
+ checkTokenMu.Unlock()
go func() {
+ defer func() {
+ checkTokenMu.Lock()
+ checkTokenInFlight = nil
+ checkTokenMu.Unlock()
+ close(done)
+ }()
+
const checkTokenBase = 1 * time.Second
const maxTokenBackoff = 5 * time.Minute
tokenBackoff := backoff.New(checkTokenBase, maxTokenBackoff)
ticker := time.NewTicker(checkTokenBase)
defer ticker.Stop()
- defer close(done)
for {
...
}
}()
return done
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orbit/cmd/desktop/desktop.go` around lines 392 - 395, The auth retry path is
starting its own checkToken() backoff while the token watcher can also start
checkToken(), causing duplicate backoff loops; change the logic so only one
in-flight checkToken() is ever started and all callers wait on its returned done
channel. Implement a shared in-flight indicator (e.g., a package-scoped variable
or small single-flight wrapper) that holds the current done channel returned by
checkToken(); on DesktopSummary auth failure and in the token watcher, check
that indicator and either reuse/wait on the existing done channel or atomically
set and start a new checkToken() and store its done channel until it completes,
then clear the indicator. Ensure synchronization (mutex/atomic) around the
shared indicator so concurrent callers race to reuse the same in-flight
checkToken() rather than spawning duplicates.
lucasmrod
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #45624
Part 1 of #45553 -- see there for the full behavioral contract and Oracle.
Changes
orbit/pkg/backoffpackage: shared, stateful exponential backoff tracker with jitter, thread-safe, per-path isolation. This package will serve all agent components that need backoff (orbit API, fleetd paths, and potentially osquery TLS), but for now only Fleet Desktop uses it. We are introducing it incrementally to reduce risk.checkTokenretry loop -- the exact tight-retry path that caused the Orbit clients hitting /device/{token}/desktop with expired tokens causes high DB usage #44816 DB outage. The main ping/DesktopSummary loop does not need backoff (Ping is unauthenticated with no DB cost; DesktopSummary already runs at most every 5 min).Manual testing
Automated tests (17 total, all pass with -race)
```
go test ./orbit/pkg/backoff/ -v -race -count=1 # 17 tests, 0 failures
make lint-go-incremental # 0 issues
```
Local TUF end-to-end test (macOS)
Set up local TUF server via `tools/tuf/test/main.sh` with `SYSTEMS=macos FLEET_DESKTOP=1 GENERATE_PKG=1`. This builds orbit and Desktop from this branch, generates `fleet-osquery.pkg` with local TUF root keys. Installed the package on macOS, enrolled to a local Fleet server.
Test: corrupt token to simulate #44816 expired-token scenario
Wrote invalid token to `/opt/orbit/identifier`, then watched Desktop and orbit logs.
Desktop backoff (exponential doubling):
```
11:57:21 ERR get device URL, backing off next_retry=2.044s (1s * 2^1 + jitter)
11:57:29 ERR get device URL, backing off next_retry=4.061s (1s * 2^2 + jitter)
11:57:39 ERR get device URL, backing off next_retry=8.744s (1s * 2^3 + jitter)
```
Orbit detects and rotates the token:
```
11:57:42 INF token TTL expired, rotating token
```
Desktop recovers instantly:
```
11:57:48 DBG enabling tray items
```
Previously Desktop would have retried every 5s indefinitely (#44816). With backoff, retry intervals double each failure and recovery is immediate on the first success.
Build verification
```
go build ./orbit/cmd/desktop/ # compiles clean
go build ./orbit/cmd/orbit/ # compiles clean
```
Checklist for submitter
fleetd/orbit/Fleet Desktop