Skip to content

fix(selfupdate): resolve false-negative terminal output by splitting SLA timeouts#310

Open
baltasarblanco wants to merge 1 commit intoGentleman-Programming:mainfrom
baltasarblanco:fix/selfupdate-timeout
Open

fix(selfupdate): resolve false-negative terminal output by splitting SLA timeouts#310
baltasarblanco wants to merge 1 commit intoGentleman-Programming:mainfrom
baltasarblanco:fix/selfupdate-timeout

Conversation

@baltasarblanco
Copy link
Copy Markdown

🔗 Linked Issue

Closes #230


🏷️ PR Type

What kind of change does this PR introduce?

  • type:bug — Bug fix (non-breaking change that fixes an issue)
  • type:feature — New feature (non-breaking change that adds functionality)
  • type:docs — Documentation only
  • type:refactor — Code refactoring (no functional changes)
  • type:chore — Build, CI, or tooling changes
  • type:breaking-change — Breaking change (fix or feature that changes existing behavior)

📝 Summary

Resolves a false-negative UI failure where the pre-flight selfUpdate routine printed a red on the terminal, even though the binary successfully updated.

Root Cause:
internal/app/selfupdate.go applied a shared 7-second context timeout to the entire upgrade flow (API ping + Backup + 9MB tarball download). On moderate-latency networks, the download exceeded the budget. The context cancelled, propagating an error to spinner.Finish(false) and printing to stdout. Meanwhile, the OS buffer allowed the atomic os.Rename to succeed in the background. Because tea.WithAltScreen() hid the original terminal buffer, the user only saw the spurious after exiting the TUI.


📂 Changes

File / Area What Changed
internal/app/selfupdate.go Replaced monolithic 7s timeout with independent SLAs: 5s for API discovery (selfUpdateCheckTimeout) and 60s for execution (selfUpdateExecTimeout). Separated context lifecycle for each phase.

🧪 Test Plan

Unit Tests

go test ./...

E2E Tests (Docker required)

cd e2e && ./docker-test.sh
  • Unit tests pass (go test ./...)
  • E2E tests pass (cd e2e && ./docker-test.sh)
  • Manually tested locally

Manual Verification Details:
Simulated a 15s download delay in upgrade_test.go confirming the success signal. Also tested manually on Ubuntu 22.04 using tc qdisc netem delay to prove the green checkmark renders correctly under network stress.


🤖 Automated Checks

The following checks run automatically on this PR:

Check Status Description
Check Issue Reference PR body must contain Closes/Fixes/Resolves #N
Check Issue Has status:approved Linked issue must have been approved before work began
Check PR Has type:* Label Exactly one type:* label must be applied
Unit Tests go test ./... must pass
E2E Tests cd e2e && ./docker-test.sh must pass

✅ Contributor Checklist

  • PR is linked to an issue with status:approved
  • I have added the appropriate type:* label to this PR
  • Unit tests pass (go test ./...)
  • E2E tests pass (cd e2e && ./docker-test.sh)
  • I have updated documentation if necessary
  • My commits follow Conventional Commits format
  • My commits do not include Co-Authored-By trailers

💬 Notes for Reviewers

Risk Assessment:

  • TUI rendering: None. Bubble Tea and tea.WithAltScreen() are untouched.
  • Platform behaviour: Low. os.Rename and context.WithTimeout are consistent across Linux, macOS, and WSL.
  • Blast radius: Confined to the timeout constants and context propagation inside selfupdate.go. Squash and merge with confidence.

…SLA timeouts

Resolves Gentleman-Programming#230.

Replaces the monolithic 7s timeout with independent SLAs for API discovery (5s) and binary execution (60s). This prevents the context from prematurely cancelling background downloads on moderate-latency networks, which previously left a spurious failure marker in the AltScreen buffer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tui): upgrade shows ✗ failure marker but binary is correctly updated

1 participant