Repository navigation
fix(update): wait briefly for the background update check before exiting - #205
Conversation
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔍 Full review · 4 files reviewed
Verification
state.Updateholds a package mutex while it reloads the file, soMarkNotifiedrunning alongside the new goroutine cannot drop each other's fields.UpdateCheck.Waithandles a nil receiver, sowaitForUpdateCheckis safe on completion paths, help paths and paths where no check was started.- The removed
CheckForUpdatehas no remaining callers in the Go code. StartUpdateCheckkeeps theGitHubRepo != ""gate (throughshouldCheckForUpdate) that the old call got from its enclosingifblock.
The new TestUpdateCheckWait covers only the nil, finish and timeout cases of Wait. No test covers StartUpdateCheck, the root-command wiring, or what happens when the GitHub request runs longer than the wait. The CI test/lint jobs (make test / make lint) cover these files only to that extent.
Review details
- Commit: 7038fcf
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental · 1 file reviewed
Verification
LastCheckedis now written synchronously, before the goroutine starts, so a request that outlives the process no longer leaves the check due on the next run.- The background
state.Updatenow sets onlyKnownLatestVersion, and only when the release lookup succeeds with a non-empty tag, so a failed lookup no longer touches the cached version. state.Updatereloads the state under a mutex, so the background write ofKnownLatestVersioncannot drop theLastCheckedorLastNotifiedwritten on the main goroutine.
No test in this increment covers the new ordering: nothing asserts that LastChecked is saved before the request, or that a failed request leaves KnownLatestVersion alone. The repository's Go test job (make test) runs only the existing state and notification tests.
Review 2 of 10 for this pull request · View the full run
Superseded: the latest Upsun Dispatch review no longer requests changes.
Short commands such as `--version` exited before the update check could save its result to the state file, so the check was lost. - Decide synchronously whether a check is due, and only then start it in the background (`internal.StartUpdateCheck`). - Before exiting, wait up to 1 second for a running check: after the command, in `exitWithError`, and after `--version` (the check now starts before `--version` is handled). - Add a 5 second timeout to the GitHub request. - Drop the unused version comparison from the check; the notice compares versions when it reads the cached result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Previously LastChecked was saved only after the GitHub request returned. On a network where the request takes longer than the 1 second wait, the process exited first, so every run started a new check and waited the full second. Saving the attempt first limits this to once per check interval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If the state file is not writable, LastChecked could not be recorded, so every run would start a new check and wait for it. The result could not be cached either, so the check is skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d848d1 to
97471ff
Compare
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔍 Full review · 4 files reviewed
🔵 Minor point
commands/root.go:71— Not every exit path callswaitForUpdateCheck.commands/config_install.go:108callsos.Exit(1)when the user answers no at the confirmation prompt, andexitSilentlyincommands/completion.goalso exits directly (completion requests skip the check, so that one is harmless). If a check is due on aconfig:installrun that ends at the prompt,LastCheckedis already saved but the request is killed. No result is cached, and nothing retries until the check interval passes. That is the same lost-check problem this PR sets out to fix.
Verification
state.Updateholds a package mutex while it reloads and saves, so the goroutine'sKnownLatestVersionwrite cannot dropMarkNotified'sLastNotified, which now runs at the same time.StartUpdateCheckruns after the completion/internal-command early return in PersistentPreRun, so completions never start a check or wait for one.UpdateCheck.Waitreturns at once on a nil receiver, sowaitForUpdateCheckis safe when no check was started, including the help paths that skip PersistentPreRun.CLIWrapper.Execruns a child process and returns; it does not call syscall.Exec. SoExecuteandexitWithErrorreally do get to wait after legacy commands.
The new TestUpdateCheckWait tests only the Wait timeout behaviour. No test covers StartUpdateCheck: not the order of the LastChecked write, not the interval gating, not the root-command wiring. Those depend on the manual PTY check described in the PR.
Review details
- Commit: 97471ff
- Model: claude-opus-5-5
Review 4 of 10 for this pull request · View the full run
…lined Declining the confirmation prompt called os.Exit(1) directly, skipping the wait for the background update check. Return an exitError instead, so Execute waits before exiting with code 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The update check runs in a background goroutine. Short commands such as
--versionexited before it could save its result tostate.json, so the check was lost.internal.StartUpdateCheck), and only then start it in the background.exitWithError, and after--version(the check now starts before--versionis handled). The wait only happens when a check is due, which is at most once per check interval (1 hour by default).LastCheckedbefore the request starts, so a slow or blocked network costs the wait at most once per check interval.Verified by running
upsun --versionin a PTY with an empty home:mainwrites no state, this branch records the latest version.🤖 Generated with Claude Code