fix(trusty-installer): stop tctl upgrade double-installing daemons, and make its restart real (#4964 phases 0+1)

crates/trusty-common/changelog.d/4964-install-location-phase-0-1.md

@@ -0,0 +1,11 @@
+Fixed
+
+- Added `bin_resolve::canonical_bin_dir` (and its pure
+  `canonical_bin_dir_from`) — one implementation of "where does `cargo install`
+  put binaries", `$CARGO_HOME/bin` falling back to `~/.cargo/bin`. Five call
+  sites across this crate and `trusty-installer` each restated the rule, and two
+  restated it wrongly: one hardcoded `~/.cargo/bin` and never read `CARGO_HOME`,
+  another treated `CARGO_HOME=""` as a real value and resolved the relative path
+  `bin`. `update::candidate_bin_dirs` now derives its first entry from the
+  shared helper rather than its own copy
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))

crates/trusty-common/src/bin_resolve.rs

@@ -328,6 +328,55 @@ pub fn is_ephemeral_build_path(path: &Path) -> bool {
     EPHEMERAL_PATH_SEGMENTS.iter().any(|seg| s.contains(seg))
 }
 
+/// The cargo binary install directory: `$CARGO_HOME/bin`, falling back to
+/// `~/.cargo/bin`.
+///
+/// Why (#4964): five call sites across `trusty-installer` and this crate each
+/// re-derived this same rule, and two of them got it wrong in the same way —
+/// they hardcoded `~/.cargo/bin` and never read `CARGO_HOME`, so a machine with
+/// `CARGO_HOME` set resolved a directory `cargo install` does not write to.
+/// One implementation means a `CARGO_HOME`-blind copy cannot be reintroduced by
+/// a fifth caller.
+///
+/// What: reads `CARGO_HOME` from the process environment and delegates to the
+/// pure [`canonical_bin_dir_from`]. Returns `None` only when `CARGO_HOME` is
+/// unset/empty AND the home directory cannot be resolved. Never spawns `cargo`
+/// — the resolution is pure path arithmetic, so it works on a machine with no
+/// Rust toolchain installed.
+///
+/// Test: `canonical_bin_dir_from_*` cover the rule; this wrapper is the
+/// side-effecting env read.
+pub fn canonical_bin_dir() -> Option<PathBuf> {
+    canonical_bin_dir_from(
+        dirs::home_dir().as_deref(),
+        std::env::var("CARGO_HOME").ok().as_deref(),
+    )
+}
+
+/// Pure resolution of the cargo binary install directory.
+///
+/// Why: extracting the rule from the env/home reads makes it testable without
+/// mutating process-global state, so the tests stay safe under the parallel
+/// harness. It is also what lets [`crate::update::candidate_bin_dirs`], which is
+/// already parameterised over explicit `home`/`cargo_home` inputs, share the
+/// same rule rather than restating it.
+///
+/// What: `<cargo_home>/bin` when `cargo_home` is `Some` and non-empty;
+/// otherwise `<home>/.cargo/bin`; `None` when neither input can supply a path.
+/// An empty `CARGO_HOME` is treated as unset — that is what cargo itself does,
+/// and treating it literally would resolve to the relative path `bin`.
+///
+/// Test: `canonical_bin_dir_from_honours_cargo_home`,
+/// `canonical_bin_dir_from_falls_back_to_dot_cargo`,
+/// `canonical_bin_dir_from_treats_empty_cargo_home_as_unset`,
+/// `canonical_bin_dir_from_is_none_without_either_input`.
+pub fn canonical_bin_dir_from(home: Option<&Path>, cargo_home: Option<&str>) -> Option<PathBuf> {
+    match cargo_home {
+        Some(h) if !h.is_empty() => Some(PathBuf::from(h).join("bin")),
+        _ => home.map(|h| h.join(".cargo").join("bin")),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -658,4 +707,46 @@ mod tests {
             "a non-existent explicit path must resolve to None"
         );
     }
+
+    /// Why (#4964): the whole reason this helper exists is that two of the
+    /// five copies it replaces ignored `CARGO_HOME` and hardcoded
+    /// `~/.cargo/bin`. A `CARGO_HOME` that is honoured is the load-bearing
+    /// behaviour.
+    /// What: a non-empty `CARGO_HOME` wins over `home` entirely.
+    /// Test: this is the test.
+    #[test]
+    fn canonical_bin_dir_from_honours_cargo_home() {
+        let got = canonical_bin_dir_from(Some(Path::new("/home/u")), Some("/opt/ch"));
+        assert_eq!(got, Some(PathBuf::from("/opt/ch/bin")));
+    }
+
+    /// Why: with no `CARGO_HOME`, cargo installs into `~/.cargo/bin`.
+    /// What: `None` cargo_home falls back to `<home>/.cargo/bin`.
+    /// Test: this is the test.
+    #[test]
+    fn canonical_bin_dir_from_falls_back_to_dot_cargo() {
+        let got = canonical_bin_dir_from(Some(Path::new("/home/u")), None);
+        assert_eq!(got, Some(PathBuf::from("/home/u/.cargo/bin")));
+    }
+
+    /// Why: `CARGO_HOME=""` is how a shell exports a variable it never set a
+    /// value for. Taken literally it resolves to the RELATIVE path `bin`,
+    /// which would place binaries under the process's working directory.
+    /// What: an empty `CARGO_HOME` resolves the same as an absent one.
+    /// Test: this is the test.
+    #[test]
+    fn canonical_bin_dir_from_treats_empty_cargo_home_as_unset() {
+        let got = canonical_bin_dir_from(Some(Path::new("/home/u")), Some(""));
+        assert_eq!(got, Some(PathBuf::from("/home/u/.cargo/bin")));
+    }
+
+    /// Why: with neither input there is no defensible guess; callers must see
+    /// `None` and decide, rather than receive a fabricated relative path.
+    /// What: both inputs absent → `None`.
+    /// Test: this is the test.
+    #[test]
+    fn canonical_bin_dir_from_is_none_without_either_input() {
+        assert_eq!(canonical_bin_dir_from(None, None), None);
+        assert_eq!(canonical_bin_dir_from(None, Some("")), None);
+    }
 }

crates/trusty-common/src/update/upgrade.rs

@@ -206,10 +206,14 @@ pub async fn perform_upgrade_captured(crate_name: &str) -> anyhow::Result<()> {
 /// `dirs::home_dir()` / `std::env::var` directly — keeps it testable without
 /// mutating global process state for the common case.
 ///
-/// What: Returns, in priority order: `<cargo_home>/bin` when `cargo_home` is
-/// `Some` and non-empty, else `<home>/.cargo/bin`; then `<home>/.local/bin`
-/// (the prebuilt installer's default). Entries that require `home` are
-/// omitted when `home` is `None`.
+/// What: Returns, in priority order: [`crate::bin_resolve::canonical_bin_dir_from`]
+/// (`<cargo_home>/bin` when `cargo_home` is `Some` and non-empty, else
+/// `<home>/.cargo/bin`); then `<home>/.local/bin` (the prebuilt installer's
+/// default). Entries that require `home` are omitted when `home` is `None`.
+///
+/// #4964: the first entry's rule is no longer restated here — it is the shared
+/// [`crate::bin_resolve::canonical_bin_dir_from`], so this list and every
+/// installer write path resolve the cargo bin dir identically.
 ///
 /// Test: `candidate_bin_dirs_prefers_cargo_home_override`,
 /// `candidate_bin_dirs_falls_back_to_dot_cargo`,
@@ -221,13 +225,8 @@ pub(crate) fn candidate_bin_dirs(
 ) -> Vec<std::path::PathBuf> {
     let mut dirs = Vec::new();
 
-    match cargo_home {
-        Some(h) if !h.is_empty() => dirs.push(std::path::PathBuf::from(h).join("bin")),
-        _ => {
-            if let Some(home) = home {
-                dirs.push(home.join(".cargo").join("bin"));
-            }
-        }
+    if let Some(canonical) = crate::bin_resolve::canonical_bin_dir_from(home, cargo_home) {
+        dirs.push(canonical);
     }
 
     if let Some(home) = home {

crates/trusty-installer/changelog.d/4964-install-location-phase-0-1.md

@@ -0,0 +1,49 @@
+Fixed
+
+- `tctl upgrade` no longer installs every daemon member twice. On the prebuilt
+  path the binary was already on disk when the daemon branch went on to call
+  `upgrade_and_restart`, whose first step is `cargo install <crate> --locked` —
+  a second copy, in a second directory, from one command. The comment saying
+  that step was "a no-op if the binary is already current" was wrong: cargo
+  skips only when its own `.crates2.json` records that exact version, and the
+  prebuilt path writes no cargo metadata. Six of the seven stable-set members
+  are daemons, so this fired on nearly every upgrade — and on a machine with no
+  Rust toolchain it errored out after the new binary had already landed
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))
+- `tctl upgrade` now actually restarts a daemon member. `upgrade_and_restart`
+  restarts by calling `std::process::exit(1)` and letting launchd's `KeepAlive`
+  respawn the process that just exited — correct for `trusty-search upgrade`,
+  `trusty-memory upgrade`, and the two MCP `upgrade` tools, which all run inside
+  the supervised daemon, and a guaranteed no-op for `tctl`, a terminal process
+  launchd has never heard of. The supervision check evaluated `tctl`, returned
+  false every time, and the manual-restart hint it produced was reported as
+  success, so the daemon kept serving the old process indefinitely. Both daemon
+  branches now bounce the member through the same launchd path `tctl restart`
+  uses (port-guard, then `bootout`, then `bootstrap` — never `kickstart -k`).
+  Note the bounce re-execs whatever path the plist's `ProgramArguments[0]`
+  names, and nothing yet rewrites that — so on a host whose plist was baked
+  from the other bin directory the daemon comes back on the same old binary.
+  Phase 4 of the epic regenerates the plist; this change stops the daemon being
+  left un-bounced, it does not yet guarantee which binary it comes back on
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))
+- `tctl install` passes the concrete path of the binary it just wrote to a
+  member's `service install`, instead of a bare name resolved through `$PATH`.
+  The spawned process bakes its own `current_exe()` into the launchd plist's
+  `ProgramArguments[0]`, so a stale copy winning the `PATH` lookup persisted
+  that stale path into launchd, which then respawned it at every boot with
+  nothing to rewrite the plist
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))
+- The component table's size column reads the binary this run just placed. It
+  joined the binary name onto the cargo bin dir while the prebuilt path writes
+  elsewhere, so it reported a stale copy's bytes, or zero
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))
+- `install_all`'s install-directory fallback reads `CARGO_HOME`. It hardcoded
+  `~/.cargo/bin` while the sibling fallback in `install_one` — same job, same
+  file — did read it. Both, plus `tctl sign`'s `--dir` default,
+  `tctl self-update`'s cargo-destination check, and `tctl upgrade`'s health-gate
+  path, now share `trusty_common::bin_resolve::canonical_bin_dir`
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))
+- Corrected the claim that `~/.local/bin` is preferred "to avoid cdhash issues
+  on macOS". What keeps the cdhash cache consistent is the atomic rename in the
+  download layer, which holds in any directory
+  ([#4964](https://github.com/bobmatnyc/trusty-tools/issues/4964))