Skip to content

Use idle read timeout for updates instead of total deadline#908

Closed
mariusvniekerk wants to merge 1 commit into
mainfrom
middleman/issue-895-update-times-out-after-30-seconds
Closed

Use idle read timeout for updates instead of total deadline#908
mariusvniekerk wants to merge 1 commit into
mainfrom
middleman/issue-895-update-times-out-after-30-seconds

Conversation

@mariusvniekerk

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes Update times out after 30 seconds #895: roborev update aborted with context deadline exceeded partway through downloading a release asset on slow connections.
  • The update HTTP client used http.Client.Timeout = 30s, an overall deadline covering the entire request including the body download — a ~12MB asset can't finish within 30s on slow wifi even when data is flowing steadily.
  • Replaced the overall timeout with a per-read idle timeout: a custom transport wraps each connection and refreshes the read deadline before every Read, so a transfer fails only when no data has arrived for 30s (a dropped connection). Slow-but-progressing downloads now succeed.
  • Kept bounded setup timeouts (dialTimeout, TLSHandshakeTimeout) so unreachable hosts still fail fast.
  • Added tests covering the no-overall-deadline guard, idle-timeout abort on a stalled connection, and success on a slow-but-steady trickle.

The update client set http.Client.Timeout to 30s, which is an overall
deadline covering the entire request including the body download. On a
slow connection a large release asset (~12MB) cannot finish within 30s
even when data is flowing steadily, so the download aborts with
"context deadline exceeded".

Replace the overall timeout with a per-read idle timeout: a custom
transport wraps each connection and refreshes the read deadline before
every Read, so a transfer fails only when no data arrives for 30s
(a dropped connection). Slow-but-progressing downloads now succeed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (109b7f6)

Summary verdict: One medium correctness risk remains in update metadata requests.

Medium

  • internal/update/update.go:165 - Removing http.Client.Timeout makes update discovery unbounded because CheckForUpdate and its fallback still use context.Background(). Small metadata, checksum, or header responses can now keep roborev update or the TUI update check alive indefinitely if a server or proxy trickles bytes within the idle timeout, even though only large asset downloads need no total deadline.

    Fix: use a bounded context or a separate timeout-enabled client for check and fallback metadata requests, while keeping idle-only behavior for PerformUpdate asset downloads.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 3m37s), codex_security (codex/security, done, 28s) | Total: 4m12s

@wesm

wesm commented Jun 28, 2026

Copy link
Copy Markdown
Member

oops I fixed this in a separate PR, is there more good stuff to include from here, worth rebasing?

@wesm

wesm commented Jun 28, 2026

Copy link
Copy Markdown
Member

looking

@wesm

wesm commented Jun 28, 2026

Copy link
Copy Markdown
Member

I think easiest to just close this, and if more issues come up we can address then

@wesm wesm closed this Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Update times out after 30 seconds

2 participants