feat(update): add daemon-driven CLI self-update - #299
Open
AnnatarHe wants to merge 3 commits into
Open
Conversation
Four defects found while building auto-update on top of `shelltime update`.
1. `.bak` meant two opposite things. ReplaceBinary writes the PREVIOUS binary
to "<dest>.bak", but commands/daemon.install.go reads that same filename as
"a NEWER daemon, restore it". So replacing the daemon and then running
`daemon install`/`daemon reinstall` restores the binary that was just
replaced. This already misfires in `shelltime update` on curl installs, and
would become an endless re-download loop once the daemon checks daily.
Introduce ReplaceBinaryWithBackupSuffix and a distinct ".prev" suffix for
update paths, so the installer's recovery branch can never fire on an
update backup. ReplaceBinary keeps ".bak" for existing callers.
2. The swap left a window with no binary. rename(dest, dest.bak) followed by
rename(tmp, dest) means dest does not exist between the two calls, and the
shell hook execs `shelltime` by name on every command -- a hook landing in
that window prints "command not found". Copy the old binary to the backup
instead, stage the new one alongside dest, and activate with a single
rename(2). Windows keeps the old order, since it cannot rename over a
running .exe.
3. DetectInstallKind misclassified Intel-mac Homebrew. goreleaser publishes a
Cask, so an EvalSymlinks'd path is /usr/local/Caskroom/... on Intel, which
matches neither the /Cellar/ check nor the /opt/homebrew/ prefix. Those
users were treated as "unknown location" and could have had the Caskroom
written into. Match /Caskroom/ explicitly.
4. mergeConfig omitted Storage entirely, so `storage: {engine: bolt}` in
config.local.yaml was silently dropped. Add it, and remove the duplicated
LogCleanup branch.
Also add CompareVersions and RestoreBinaryBackup, and fail DownloadAndVerify
when the body is shorter than Content-Length -- both are needed by the
unattended update path, where a downgrade or a truncated download must never
reach the swap step. ReleaseSource lands here because FetchChecksum is
refactored to route through it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Users sit on stale CLIs because updating is a manual step they never take, and users behind a GitHub block cannot update at all. Have the long-lived daemon check once a day and install updates through the shelltime API. Daemon (daemon/auto_update.go): - Wakes hourly and gates on a PERSISTED LastCheckAt rather than running a 24h ticker. A 24h ticker resets on every daemon restart, so a daemon that restarts more often than that would never check at all; the persisted gate self-corrects across restarts and makes a crash loop harmless. Jittered so a fleet does not stampede the server after a release. - Updates the CLI binary in place but only STAGES the daemon binary at bin/shelltime-daemon.next. The running daemon must not rewrite the binary it is executing, and staging means the shell-side repair is a local rename with no network call inside the hook and no second 15MB download. - Homebrew is opt-in (autoUpdate.homebrew); by default we only record a notice, because brew can prompt and touches files we do not own. When it does run, stdin is closed and NONINTERACTIVE is set so it can never block. CLI drift repair: - `track` runs on every command, so it gets a 1-in-64 sampled check: a constant compare plus rand.IntN, under 100ns with zero syscalls on the common path, and one stat(2) of a normally-absent marker when sampled. Uses math/rand/v2, whose global source is seeded per process -- every `track` is a fresh process, so a v1-style fixed seed would make them all sample identically. - `gc` runs once per new shell and carries the deterministic check. It is also the only place a notice can print without interleaving with command output. - The repair itself runs detached via a hidden `daemon apply-update`, in its own session, so launchctl/systemctl work cannot stall the prompt and Ctrl-C cannot kill a half-finished swap. Keeping the daemon alive: - shouldReinstallDaemon bailed when installer.Check() failed -- precisely when the service was NOT running -- so `shelltime update` left a stopped daemon stopped forever. Since the daemon drives the update check, that killed auto-update permanently. It now only decides whether we manage a daemon at all; whether it is running picks reinstall-vs-install. - ensureDaemonRunning verifies the service actually came up by polling both the service manager and the socket, rather than trusting StartService()'s exit code, which only reports that launchctl accepted the command. - Every exit path of `daemon apply-update` from the staging check onward calls it -- including when the smoke test fails or the swap is rolled back. An unreachable daemon means repair, not bail. - `gc` also restarts a long-down daemon, closing the chicken-and-egg where a dead daemon means no check ever runs again. Guarded on the service file still existing, so a deliberate `daemon uninstall` is left alone, and rate limited to once an hour. Safety: downloads are checksum-verified, cross-checking the API-provided sum against checksums.txt; the unattended path fails closed when it cannot verify, while interactive `shelltime update --allow-unverified` may opt out. Every binary must print its own version before AND after installation, with automatic rollback. CompareVersions guarantees we never downgrade. Dev builds are skipped, unwritable and unrecognized locations only produce a notice (never sudo), a shared lock serializes the daemon and every CLI repair, and SHELLTIME_DISABLE_AUTO_UPDATE stops everything without a config edit. Enabled by default (autoUpdate.enabled), following the logCleanup convention. Note the trust root is TLS to api.shelltime.xyz plus a checksum that same server supplies -- better than trusting github.com the same way, but not signature verification. Release signing is the follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (52.20%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
`track` runs inside the shell hook on every command, so it must never do update work. Previously it sampled a marker file and, on a hit, spawned `daemon apply-update` -- which swaps the daemon binary. Even though the download itself always happened in the daemon, binary-swapping is far more than a per-command hook should ever be responsible for. The most track may now do is start a stopped daemon service, and it ignores whether that succeeds. Move the check to where track has ALREADY established that no daemon is reachable, after both the default and the configured socket have failed. That makes it free on the happy path: a running daemon returns long before reaching it, so the common case adds no rand, no stat, and no syscall at all -- strictly cheaper than the previous top-of-function sampling. Applying a staged update stays in `gc`, which runs once per new shell rather than once per command, and downloading stays exclusively in the daemon. Add a structural test that parses the call graph out of commandTrack and fails if it can reach any download/extract/verify/swap helper. A behavioural test could only show that today's path is clean; this shows nobody can reintroduce update work without deleting the test. Verified it fails on an injected violation and names the offending call path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Why
Two gaps in the existing manual
shelltime update:The daemon now checks once a day, pulls binaries through
api.shelltime.xyz, replaces the CLI, and stages the daemon; a later shell finishes the swap and restarts the service.Requires shelltime/server#462 for the API and download proxy. Without it the CLI falls back to the GitHub path it uses today, so this is safe to merge in either order.
Where each responsibility lives
This split is the core of the design — worth checking first:
gc(once per new shell)track(every command)trackruns inside the shell hook on every command, so it never downloads, extracts, verifies, or swaps a binary. The most it may do is start a stopped daemon, and it ignores whether that succeeds.That check sits after track has already failed to reach a daemon on both the default and configured sockets, so it is free on the happy path — a running daemon returns long before reaching it, adding no rand, no stat, and no syscall to the common case.
TestCommandTrack_NeverPerformsUpdateWorkparses the call graph out ofcommandTrackand fails if it can reach any update helper. A behavioural test could only show today's path is clean; this shows nobody can reintroduce update work without deleting the test. (Verified it fails on an injected violation and names the offending path.)Other design notes worth reviewing
Hourly ticker + persisted
LastCheckAt, not a 24h ticker. A 24h ticker resets on every daemon restart, so a daemon restarting more often than that would never check at all. The persisted gate self-corrects across restarts and makes a crash loop harmless. Jittered so a fleet does not stampede after a release.The daemon stages, the CLI activates. The daemon replaces the CLI binary but only stages the daemon binary at
bin/shelltime-daemon.next. The running daemon must not rewrite the binary it is executing, and staging makes the shell-side repair a local rename — no network call, no second 15MB download.Repair runs detached.
launchctl/systemctltake hundreds of ms to seconds and print progress. Running that inline would stall the prompt. The hiddendaemon apply-updateruns in its own session, so Ctrl-C or the shell exiting cannot kill a half-finished swap.Bugs fixed along the way
The first commit is standalone (builds and tests on its own) and worth reviewing separately — these are live on
maintoday:.bakmeant two opposite things.ReplaceBinarywrites the old binary there;daemon.install.go:33-40reads it as "a newer daemon, restore it". Soshelltime updateon a curl install has been silently rolling the daemon back — and with a daily check that becomes an endless re-download loop. Fixed with a distinct.prevsuffix.shelltimedid not exist.rename(dest, dest.bak)thenrename(tmp, dest)leaves the binary absent between two syscalls, and the hook execs it by name on every command. Now a single atomicrename(2)over the destination.shouldReinstallDaemonbailed wheninstaller.Check()failed — precisely when the service was down. Since the daemon drives the update check, that killed auto-update permanently./usr/local/Caskroom/...matched neither/Cellar/nor the/opt/homebrew/prefix — those users were treated as "unknown location".mergeConfigdroppedstorageentirely, sostorage: {engine: bolt}inconfig.local.yamlwas silently ignored.Keeping the daemon alive
Every path that touches daemon binaries ends in
ensureDaemonRunning, which verifies the service came up (polling the service manager and the socket) rather than trustingStartService()'s exit code — that only reports thatlaunchctlaccepted the command. This holds even when the smoke test fails or the swap is rolled back. An unreachable daemon means repair, not bail.Both
gcandtrackwill restart a long-down daemon, closing the chicken-and-egg where a dead daemon means no check ever runs again — guarded on the service file still existing (so a deliberatedaemon uninstallis left alone) and rate limited to once an hour.Safety
Checksum-verified with the API sum cross-checked against
checksums.txt; the unattended path fails closed when it cannot verify, while interactive--allow-unverifiedmay opt out. Every binary must print its own version before and after installation, with automatic rollback. Never downgrades. Dev builds skipped. Unwritable and unrecognized locations only produce a notice — the daemon never usessudo. A shared lock serializes the daemon and every CLI repair.SHELLTIME_DISABLE_AUTO_UPDATE=1stops everything without a config edit.Enabled by default (
autoUpdate.enabled), following thelogCleanupconvention. Docs indocs/CONFIG.md.Caveat worth stating: the trust root is TLS to
api.shelltime.xyzplus a checksum that same server supplies. Better than today (which trusts github.com the same way), but not signature verification — a compromised server can serve a malicious binary. Release signing (cosign/minisign, public key pinned in the binary) is the follow-up.Testing
go build ./...clean;go vetshows only the pre-existingcmd/cli/main.go:25finding.New tests cover the atomic swap (asserting the destination never disappears), the
.bakcollision regression, downgrade refusal, checksum disagreement, proxy→GitHub fallback, rollback on a broken binary, lock exclusivity under concurrency, track's start-only restriction and its rate limiting, and — forapply-update— that the daemon is restarted even when the smoke test or the swap fails.Full-suite failures were diffed against
mainand each remaining one re-checked in isolation on both branches: no new failures. The pre-existing ones are macOS unix-socket path-length limits and PostgreSQL-dependent suites.🤖 Generated with Claude Code