Orbit file download stall timeout and 30s context timeout - #50984
Conversation
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds configurable stall detection to Possibly related PRs
Mergeability Score: ⚪ Minimal · up to The PR adds context-timeout and download-stall timeout handling with automated tests; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
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 `@client/orbit_client.go`:
- Line 527: Update both file-download request paths around
requestWithExternal/DoHTTPRequest to use a context with a bounded pre-body
deadline instead of context.Background(). Apply the deadline through
response-header/TLS setup while preserving the existing watchdog and allowing
the response body to download without that deadline limiting its duration.
In `@orbit/changes/46801-context-timeout-stall-file-download`:
- Line 1: Restrict the 30-second context timeout to authenticated non-file orbit
requests, and exclude file-download requests from that timeout path. Preserve
the existing stall watchdog as the timeout mechanism for file downloads.
🪄 Autofix
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 Plus
Run ID: b141b73e-cb28-4f91-97f8-5f9a32c41a9b
📒 Files selected for processing (4)
client/base_client.goclient/base_client_stall_test.goclient/orbit_client.goorbit/changes/46801-context-timeout-stall-file-download
There was a problem hiding this comment.
Pull request overview
Adds request/download timeouts in the Orbit client to prevent setup experience hangs caused by stalled network transfers (e.g., network filters) and to bound request latency.
Changes:
- Introduces a per-download “stall timeout” watchdog that aborts downloads when no bytes are received for a configured duration.
- Adds logic intended to apply a 30s context timeout to Orbit HTTP requests (with special handling for download paths).
- Adds unit tests to validate stalled vs slow-but-progressing download behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| orbit/changes/46801-context-timeout-stall-file-download | Documents the new 30s request timeout and 60s download stall timeout behavior. |
| client/orbit_client.go | Threads request contexts and attempts to apply a 30s timeout while exempting download flows. |
| client/base_client.go | Implements stall watchdog logic in FileResponse.Handle to abort stalled downloads. |
| client/base_client_stall_test.go | Adds tests covering stalled download aborts and slow-progressing downloads completing successfully. |
Suppressed comments (1)
client/orbit_client.go:770
authenticatedRequestcurrently special-cases*FileResponse, but other streaming/custom body handlers (e.g.*NullFileResponseused for alt=media downloads) will still get the 30s deadline and can be canceled even when they are slow-but-progressing. Ifrequest()is updated to enforce a 30s timeout by default, consider opting out for allBodyHandlerresponses by routing them throughrequestWithContext(context.Background(), ...)(no deadline) so streaming downloads rely on the stall watchdog instead of a fixed overall timeout.
s := params.(fleet.SetOrbitNodeKeyer)
s.SetOrbitNodeKey(nodeKey)
if _, ok := resp.(*FileResponse); ok {
// Handle file response here if needed
err = oc.request(verb, path, params, resp)
} else {
// if not FileResponse, set context with timeout of 30 seconds for all other requests.
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
err = oc.requestWithContext(ctx, verb, path, params, resp)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #50984 +/- ##
==========================================
+ Coverage 68.53% 68.63% +0.09%
==========================================
Files 3980 3995 +15
Lines 256315 257583 +1268
Branches 13661 13661
==========================================
+ Hits 175676 176794 +1118
- Misses 65005 65060 +55
- Partials 15634 15729 +95
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
GitHub Actions
Inactive
cdcme
left a comment
cdcme
left a comment
There was a problem hiding this comment.
Just a few questions, looks good overall.
|
Thanks @cdcme I went through your comments and either made fixes or left a comment for you to look at |
GitHub Actions
Inactive
GitHub Actions
Inactive
Related issue: Resolves #46801
The 30s context timeout can be tested with the package in the issue. The speculative software download stall timeout I was unable to repro with actual packages/files.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit