Keep the on-$PATH miren CLI in sync with the self-managed server#969
Conversation
The install docs put the miren binary on $PATH at /usr/local/bin/miren and tell you to use it directly. But `miren server upgrade` only advances the managed payload at /var/lib/miren/release/miren; it never touched the CLI the docs told you to install, so after the first upgrade the CLI on $PATH silently drifts from the running server. We hit exactly this on one of our own clusters: the server was current while /usr/local/bin/miren was a stale build missing newer commands. Since miren self-manages the deploy, it should own the on-$PATH CLI too. `server install` and `server upgrade` (which also covers runner and coordinator upgrades) now maintain /usr/local/bin/miren as a symlink to the managed release binary. A symlink is self-correcting: it targets the path, so it tracks whatever install or rollback last put there, and because upgrades swap the binary via atomic rename a miren invoked through the symlink keeps its mapped inode. It's best-effort after a healthy upgrade, so a hiccup never fails or rolls back a good server, and we leave a foreign symlink (e.g. a Homebrew Cask) alone rather than stepping on another tool.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a release utility that creates, refreshes, and atomically replaces the managed CLI symlink while preserving foreign symlinks. Server upgrades and installations now invoke this utility as a best-effort post-operation step. Upgrade configuration includes a Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
This is a draft, so I'm judging readiness to hand off to human review. The core logic is solid and I'd be comfortable with most of it, but there's one concrete gap worth flagging before this graduates.
What's here and what I checked
The PR adds pkg/release/symlink.go — a self-contained EnsurePathSymlink function — and wires it into two places: ServerInstall (linux CLI) and Manager.UpgradeServer (the upgrade path). The atomic swap via os.CreateTemp + os.Remove + os.Symlink + os.Rename is the right pattern for keeping an in-use binary's inode alive during replacement on Linux. The ErrPathManagedElsewhere sentinel and the symlinkPointsInto guard are well-reasoned: they prevent silently hijacking a Homebrew/Cask-managed link. The test coverage in symlink_test.go is thorough — it covers creation, stale-file replacement, stale-managed-symlink replacement, foreign-symlink preservation, and idempotency.
The one real gap: ServerUninstall doesn't remove the symlink
When miren server uninstall runs, it tears down the systemd service and optionally removes /var/lib/miren, but it never touches /usr/local/bin/miren. After uninstall, the symlink points at a binary that no longer exists (or will be removed). A user who then types miren gets a broken command with no obvious hint about what happened. Given that the PR specifically creates the symlink during install, cleaning it up on uninstall is the expected counterpart. It's not a security issue, but it is a correctness gap that a human reviewer will almost certainly ask about.
One minor implementation note (non-blocking)
In EnsurePathSymlink, os.CreateTemp opens a file and returns an *os.File. The code calls tmp.Close() but there's a window between Close and os.Remove where the temp file sits on disk open-file-descriptorless. This is fine on Linux (the file still exists until Remove) and the code handles it correctly — I just want to note the sequence is correct, not a bug.
Summary
The symlink mechanism itself is ready. The missing ServerUninstall cleanup is the one thing I'd want addressed (or at least consciously deferred with a TODO) before this goes to a human reviewer.
🍪 full review note · comment /biscuit review to run biscuit again.
Inline comments
cli/commands/server_install.go:558
ServerInstall creates /usr/local/bin/miren as a symlink, but ServerUninstall never removes it. After uninstalling, miren on $PATH points at a deleted binary. Consider calling os.Remove(release.SystemCLIPath) (or a dedicated release.RemovePathSymlink) during uninstall — but only when it is the symlink we own (i.e. points into releaseBinPath's directory), to avoid removing a link belonging to another tool.
🤖 Prompt for AI Agents
In cli/commands/server_install.go, the
ServerUninstall function (starting around line
558) does not clean up the /usr/local/bin/miren
symlink that ServerInstall creates. Add a step at
the end of ServerUninstall (before the final
return nil) that removes release.SystemCLIPath if
and only if it is a symlink pointing into
/var/lib/miren/release (i.e. use
release.EnsurePathSymlink's existing
symlinkPointsInto helper or an equivalent check
via os.Readlink). If the symlink belongs to
another tool, leave it alone and emit a ctx.Info
note instead. If the remove fails, emit a ctx.Warn
rather than returning an error, consistent with
the best-effort pattern used in ServerInstall.
2 participants
The install docs put the
mirenbinary on$PATHat/usr/local/bin/mirenand tell you to use it directly. That's fine until the first upgrade:miren server upgradeinstalls the new payload to/var/lib/miren/release/miren, repoints systemd, restarts, and health-checks, but it never advances the/usr/local/bin/mirenthe docs told you to install. From then on the CLI on$PATHsilently drifts from the running server. We hit exactly this on one of our own clusters, where the server was current but/usr/local/bin/mirenwas a stale build missing commands the server had.Since miren is all about self-managing a deploy, it should own getting a current CLI onto
$PATHtoo. Somiren server installandmiren server upgradenow maintain/usr/local/bin/mirenas a symlink to the managed release binary. The same path covers runner and coordinator upgrades, which is exactly where you SSH in and want a matching CLI.A symlink beats a refresh-copy because it's self-correcting: it targets the path, not an inode, so it tracks whatever
installor rollback last dropped at/var/lib/miren/release/miren, including after an auto-rollback. And because upgrades swap the binary via atomic rename, amireninvoked through the symlink keeps its already-mapped inode, so replacing the target under a running process is safe on Linux. It's maintained best-effort after a healthy upgrade, so a symlink failure warns rather than failing an otherwise-good server.On existing boxes
/usr/local/bin/mirenis a real stale file today; the helper atomically replaces it, so the first upgrade after this ships heals the drift on its own. We stay polite about it: we only take over the path when it's a plain file, missing, or already our own symlink. A symlink another tool owns (a Homebrew Cask linking into its Caskroom, say) is left alone. macOS never reaches this code anyway, since the server commands are Linux-only.Closes MIR-1275