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

[bobmatnyc]

Phases 0 and 1 of #4964. Both are destination-independent correctness fixes — they land safely regardless of the later destination flip.

Covered here: Phase 0 (0.1 canonical_bin_dir, 0.2 absolute path to service bootstrap, 0.3 the two intra-installer divergences) and Phase 1 (1a working restart, 1b stop the double-write).

Deliberately NOT covered: Phase 2 (cargo ownership conflict), Phase 3 (destination flip), Phase 4 (migration), Phase 5 (doctor check, guard script, ADR). Phases 2 and 3 must ship together, and Phase 3 is blocked behind a separate binary_provenance / ADR-0021 reconciliation — shipping any part of it here would make the tm doctor false-positive real on a daily driver. The epic stays open.

Homebrew is out of scope for the epic, as accepted residual risk (owner ruling). bobmatnyc/homebrew-trusty is live and bin.install lands in $(brew --prefix)/bin, which this repo cannot redirect.

Phase 0

0.1 — one implementation of "where does cargo install put binaries". Added trusty_common::bin_resolve::canonical_bin_dir, plus the pure canonical_bin_dir_from(home, cargo_home) that holds the rule. Five sites restated it; two restated it wrongly:

• install.rs's install_all fallback hardcoded ~/.cargo/bin and never read CARGO_HOME, while the sibling fallback in install_one — same job, same file — did.
• self_update::cargo_install_dir treated CARGO_HOME="" (how a shell exports a variable it never assigned) as a real value and resolved the relative path bin.

Also routed through it: tctl sign's --dir default, upgrade.rs's health-gate destination, install.rs's cargo_bin_dir, and trusty_common::update::candidate_bin_dirs's first entry.

0.2 — the stale-daemon respawn loop. install_all passed a bare binary name to bootstrap_member_service, which resolved it with which::which and spawned it. That process bakes its own current_exe() into the launchd plist's ProgramArguments[0], and KeepAlive respawns exactly that path at every boot — nothing rewrites the plist unless service install runs again, and re-running reproduces the same resolution. So on a host where a stale copy sits earlier on PATH, tctl install placed a new binary and persisted the old one into launchd. It now passes installed.path, the concrete path the install just wrote. tctl start, which installs nothing, keeps the PATH lookup via an explicit None.

0.3 — the two intra-installer divergences. binary_size joined a bare name onto the cargo bin dir while the prebuilt path writes elsewhere, so the component table's size column described a stale copy or reported zero; it now takes the concrete path, which makes the divergence structurally impossible on either branch and under any future destination. tctl sign's default and install.rs's helper now share canonical_bin_dir. The post-install hook's set loop still signs under install_dir — that is the directory the binaries are actually in today, and it becomes canonical for free at Phase 3.

The false comment. install.rs justified ~/.local/bin as chosen "to avoid cdhash issues on macOS". It is not. The atomic rename in download/fetch.rs is what keeps the cdhash cache consistent, and that holds in any directory. Corrected in place.

Phase 1

Both defects lived in one call, commands/upgrade.rs's upgrade_and_restart on the daemon branches.

1b — the double-write. On the prebuilt branch the binary was already on disk, and upgrade_and_restart's first step is cargo install <crate> --locked, landing 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 it fired on nearly every upgrade — and on a machine with no Rust toolchain it errored out after the new binary had landed.

What the restart does NOT yet guarantee

bootout/bootstrap re-exec whatever path the plist's ProgramArguments[0] names, and nothing on this path rewrites it. The prebuilt branch writes ~/.local/bin; the fallback branch cargo installs to $CARGO_HOME/bin. So on a host whose plist was baked from the other directory, tctl upgrade spends a downtime window bouncing the daemon and brings the same old binary back, while reporting upgraded to X; restarted.

That is a false success report. Phase 4 owns the plist regeneration that fixes it and Phase 3 removes the directory divergence that makes it reachable — neither is in scope here. What this PR changes is that the daemon is no longer left un-bounced entirely; it does not yet guarantee which binary comes back. Both upgrade.rs's module doc and the changelog fragment now say exactly that instead of promising the outcome. Separately noted in place: lifecycle::own_verb_control resolves trusty-mpm through which::which, so it can bounce a different copy than the concrete path this PR health-gates — same root cause, same Phase 3 fix.

1a — the restart. upgrade_and_restart restarts by calling std::process::exit(1) and letting launchd's KeepAlive respawn the process that just exited. That is correct for its other callers — trusty-search upgrade, trusty-memory upgrade, and the two MCP upgrade tools all run inside the supervised daemon. tctl is a terminal process launchd has never heard of, so the supervision check evaluated tctl, returned false every time, and the manual-restart hint it produced was reported as success. tctl upgrade has never restarted a daemon member.

The fix is not a restart-only variant of upgrade_and_restart — exit(1) from tctl still restarts nothing. Both daemon branches now health-gate the concrete path and then bounce the member through lifecycle::restart_member, the same path tctl restart already uses: port guard first, then bootout, then bootstrap, honouring the ExitTimeOut drain window. Never launchctl kickstart -k, which sends SIGKILL. The fallback branch gets the restart too — new behaviour there as well.

trusty_common::update::upgrade_and_restart itself is unchanged and still serves its four in-daemon callers.

Tests

Every new test was confirmed red against a deliberate break of the behaviour it names.

Test	Goes red when
bin_resolve::canonical_bin_dir_from_honours_cargo_home (+3 siblings)	the resolver ignores CARGO_HOME
service_install_target_prefers_the_concrete_path_over_path_lookup	the concrete path is dropped for a which lookup
bootstrap_one_forwards_the_concrete_exe_path_to_service_install	bootstrap_one drops the path before the spawn
binary_size_reads_the_concrete_path	binary_size goes back to joining a name onto the cargo bin dir
restart_plan_daemons_restart (+2 siblings)	any daemon class stops planning a restart
no_installer_call_site_invokes_upgrade_and_restart	the call is reintroduced anywhere under src/

restart_plan is a pure function precisely so the dispatch is testable — invoking restart_member in a unit test would bounce the host's live daemons.

upgrade.rs crossed the 500-SLOC production cap, so its tests moved to a sibling upgrade_tests.rs, the split service_bootstrap.rs already uses.

Two cargo_bin_dir_from_env_* tests were deleted from install_tests.rs — the local copy of the rule they covered no longer exists, and the same three cases (plus a fourth for the no-home case) are now in trusty-common beside the one implementation.

Gates — rung 4 (cross-crate: trusty-installer + trusty-common)

cargo fmt --check                                                    — clean
bash scripts/check_test_pointers.sh     — 22088 citations, 0 dangling
cargo check --workspace --all-targets                                — clean*
cargo clippy -p trusty-installer -p trusty-common --all-targets -D warnings — clean
cargo test -p trusty-installer          — 562 passed, 0 failed, 4 ignored
cargo test -p trusty-common             — 292 passed, 0 failed, 6 ignored
cargo test -p trusty-common --features update-check — 324 passed, 0 failed, 11 ignored
bash scripts/check_line_cap.sh          — 3743 files, 0 violations
bash scripts/check_sld.sh               — 0 errors, 0 warnings

The pointer lint was missing from this list on the first push, and it is what
went red: lifecycle.rs:269 cited daemon_restart_routes_by_manage_strategy,
a name from an earlier draft renamed to restart_plan_* when RestartPlan was
extracted. upgrade.rs:465 carried the same stale name and passed the gate only
because the parser stops extracting at the first non-backticked token — fixed
too. Two further Test: blocks led with prose and were therefore skipped
unchecked; one now leads with its citation so the gate covers it, and the other
(sign::default_bin_dir, whose only proof is cross-crate and unverifiable from
this crate) says so plainly rather than naming a test the gate cannot resolve.

* --workspace --all-targets fails on trusty-mpm-gui and trusty-code-gui in any fresh worktree: tauri::generate_context! panics because frontendDist = "ui/dist" has not been built. Pre-existing and environmental — this branch touches no file in either crate. Those two are --excluded from the local run only; CI builds the UI and covers them.

The --features update-check run is separate on purpose: trusty-common's default feature set is empty, so a bare cargo test -p trusty-common never compiles the update module at all and exits 0 having skipped candidate_bin_dirs entirely.

No consumer suites were run beyond that. The only trusty-common item this branch changes is candidate_bin_dirs, which is pub(crate) — no crate outside trusty-common can call it, and its four existing tests pin the output as unchanged. canonical_bin_dir is new, and its only consumer is trusty-installer, which is fully tested here.

Neither crate needs a version bump: trusty-installer is at 0.5.1 against 0.5.0 live, trusty-common at 0.29.0 against 0.28.1 live.

Refs #4964 — the epic stays open for phases 2 through 5.

🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools

[bobmatnyc]

Code Critic — Verdict: WARN

Adversarial review of the diff against #4964 Phases 0+1, from an isolated worktree at e9a2a9a4. Weighted on the restart path, which is rung 5.

Findings

Severity	File	Line	Issue	Fix	Disposition
HIGH	crates/trusty-installer/src/commands/lifecycle.rs	269	Test: pointer cites daemon_restart_routes_by_manage_strategy, which does not exist anywhere in the crate — the test is named restart_plan_daemons_restart. The CI-enforced doc-pointer lint is failing on this PR right now (job); I reproduced it locally. The gate list in the PR body does not include check_test_pointers.sh.	Cite super::upgrade::tests::restart_plan_daemons_restart (or restart_plan_*). Same stale name at upgrade.rs:465 — not flagged only because the parser stops at the first non-backticked token; fix both.	Fix here
MEDIUM	crates/trusty-installer/src/commands/upgrade.rs	11-12	Module doc: "Daemons are restarted after the new binary lands, so launchd-supervised members come back on it rather than continuing to serve the old one." The second clause holds only when the plist's ProgramArguments[0] already resolves inside the directory this run wrote to. Nothing on the upgrade path rewrites the plist.	Qualify: the restart re-launches whatever ProgramArguments[0] names; converging that on the just-written path is Phase 3/4.	Fix here
MEDIUM	crates/trusty-installer/src/commands/upgrade.rs	496-501, 529-536	The restart re-activates what the supervisor already points at, not the binary just placed. launchd_control(Verb::Restart) (lifecycle.rs:405-419) builds a label-only LaunchdConfig and only does bootout+bootstrap. Prebuilt writes ~/.local/bin; the fallback branch cargo installs to $CARGO_HOME/bin. On a host whose plist was baked from the other directory, tctl upgrade now spends a real downtime window bouncing the daemon and brings the same old binary back, while reporting upgraded to X; restarted <label>. Same shape at lifecycle.rs:435-448: own_verb_control resolves trusty-mpm through which::which rather than the concrete path this PR just health-gated — the exact PATH-shadow class Phase 0.2 removes from service install.	Epic Phase 4 (plist regeneration) is the mechanism fix and is correctly out of this PR. Track it as a Phase 4 precondition rather than reworking here.	Parent
LOW	PR body	—	"still serves its four in-daemon callers" — there are five call sites. The fifth, crates/trusty-mpm/src/bin/tm/commands/reinstall.rs:220, is not in-daemon: tm reinstall is a terminal process, so it carries the identical never-restarts defect Phase 1a exists to fix, and it is absent from the epic's own call-site table. No code defect here — the function is unchanged and all five compile.	Add it to the epic's inventory.	Parent

Zero CRITICAL. One HIGH → WARN. The HIGH is a red required check, so it must be green before merge regardless of verdict.

What I attacked, and what held

1. The restart path (blast radius). No path bounces a daemon that a no-op run would have left alone: the bounce is gated behind a newer version existing, user confirmation, a successful placement, and a passing concrete-path health gate. tctl install and tctl start are unchanged in what they bounce (run_service_install(binary, None) at lifecycle.rs:403 preserves the PATH lookup exactly). bootout-succeeds-then-bootstrap-fails does leave the daemon down with no supervisor rescue, but the port guard runs before the bootout (lifecycle.rs:400-410), the error string says "booted out successfully; bootstrap failed", and apply_all (upgrade.rs:399-407) surfaces it as ok:false with that message. That is the existing tctl restart contract, newly reachable — not a new fail-open.

2. The declined Phase 1b variant — the reasoning is correct. A place-nothing variant of upgrade_and_restart would keep exit(1) + is_launchd_supervised(), which checks std::process::id(); from tctl it returns false unconditionally. So the variant fixes 1b and leaves 1a broken for its one caller. What shipped instead — verify_installed_binary_at_path (already pub, already concrete-path) plus lifecycle::restart_member — satisfies both, adds no shared-library surface, and gives the prebuilt daemon branch a stronger health gate than the variant would have (concrete path, not a candidate_bin_dirs name probe that checks the cargo dir first). Declining was right.

3. upgrade_and_restart is genuinely untouched. The only edit in trusty-common/src/update/upgrade.rs is candidate_bin_dirs' body and doc, and the new body is behaviourally identical to the old match. cargo check --workspace --all-targets --exclude trusty-mpm-gui --exclude trusty-code-gui → EXIT=0, covering all five callers.

4. The consolidation is behaviour-preserving where it should be. Walked all seven old restatements against canonical_bin_dir_from. Five are byte-identical rules. Two change, both deliberately and both toward correctness: install_all's fallback now reads CARGO_HOME, and self_update::cargo_install_dir stops treating CARGO_HOME="" as real (it used to return the relative path bin). Relative CARGO_HOME still yields <relative>/bin — unchanged, and what cargo itself does. Non-unicode CARGO_HOME behaves as before (var().ok() → None matches the old unwrap_or_default() → "").

5. Test honesty — five of six breaks applied, all red exactly where named.

canonical_bin_dir_from ignores CARGO_HOME
  → canonical_bin_dir_from_honours_cargo_home FAILED (3 passed; 1 failed)

binary_size joins the name onto the cargo bin dir
service_install_target drops the concrete path
restart_plan stops special-casing !macos
trusty_common::update::upgrade_and_restart( reintroduced under src/
  → binary_size_reads_the_concrete_path                              FAILED
  → service_install_target_prefers_the_concrete_path_over_path_lookup FAILED
  → restart_plan_daemons_restart                                      FAILED
  → no_installer_call_site_invokes_upgrade_and_restart                FAILED
  test result: FAILED. 558 passed; 4 failed; 4 ignored

Each break reddens its named test and nothing else. On restart_plan's purity: what stays untested is restart_daemon_member's two-arm match and restart_member's StableMember synthesis. I read both — the Restart path reads only manage and binary; crate_name, daemon, and required are never touched by apply_to_member, launchd_control, or own_verb_control. The synthesis cannot drift into something meaningful. Purity costs nothing here.

6. Both gate claims are true.

cargo test -p trusty-common                        — 292 passed, 0 failed, 6 ignored
                                                     candidate_bin_dirs mentions in output: 0
cargo test -p trusty-common --features update-check — 324 passed, 0 failed, 11 ignored
                                                     candidate_bin_dirs mentions in output: 5

The bare run does skip the whole update module while exiting 0. Running the feature run separately was necessary, not ceremony.

The --workspace --all-targets failure is exactly two crates and exactly the stated cause:

error: proc macro panicked
   |         .run(tauri::generate_context!())
error: could not compile `trusty-code-gui` (lib) due to 1 previous error
error: could not compile `trusty-mpm-gui` (lib) due to 1 previous error

git diff --name-only origin/main...HEAD -- crates/trusty-mpm-gui/ crates/trusty-code-gui/ → 0 files.

Other gates I ran: cargo test -p trusty-installer — 562 passed, 0 failed, 4 ignored. cargo clippy -p trusty-installer -p trusty-common --all-targets -- -D warnings — EXIT=0. check_line_cap.sh — 3743 files, 0 violations. check_sld.sh — 0 errors, 0 warnings. Changelog fragments: one category each, directly in changelog.d/.

Required before merge

1. Fix the two dangling Test: pointers (lifecycle.rs:269, upgrade.rs:465) so "Test: doc-comment pointer lint" goes green.
2. Qualify the module-doc claim at upgrade.rs:11-12.

Notes

Scope honoured: trusty-agents, trusty-code, Homebrew, and Phases 2-5 were not reviewed. No daemon was restarted, nothing was installed, and no file outside .claude/worktrees/critic-5011 was touched — the four temporary test-breaks were reverted and git status --porcelain is clean.

🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools

[bobmatnyc]

Code Critic — Re-review at 245db8f0: APPROVE

Scoped to the three findings from the previous round. Everything cleared there stands and was not re-examined.

Findings

Severity	File	Line	Issue	Fix	Disposition
LOW	PR body	40	Still says upgrade_and_restart "serves its four in-daemon callers". There are five, and the fifth — crates/trusty-mpm/src/bin/tm/commands/reinstall.rs:218-220 — is not in-daemon; its own comment reads "May not return: upgrade_and_restart exits(1)", which from a terminal tm is the same never-restarts defect Phase 1a exists to fix. Not a code defect: the function is unchanged and all five compile.	Add it to the epic's call-site inventory.	Parent

Zero CRITICAL, zero HIGH → APPROVE.

Finding 1 (HIGH) — cleared, and the gate now reaches further than it did

Both pointers cite real tests and name the file they live in. Every name resolves, in the file named:

cargo_install_dir_ends_in_bin                    → self_update.rs
restart_plan_daemons_restart                     → upgrade_tests.rs
restart_plan_non_daemon_is_a_noop                → upgrade_tests.rs
restart_plan_launchd_member_off_macos_is_manual  → upgrade_tests.rs
no_installer_call_site_invokes_upgrade_and_restart → upgrade_tests.rs
applied_report_all_ok_reflects_shadow_failure    → upgrade_tests.rs
binary_size_reads_the_concrete_path              → install_tests.rs
service_install_target_prefers_the_concrete_path_over_path_lookup → service_bootstrap_tests.rs

No surviving citation of the deleted cargo_bin_dir_from_env_* tests — the one remaining mention is a plain // comment in install_tests.rs:276 explaining the move, not a Test: pointer. The daemon_restart_* hits elsewhere in the tree are trusty-mpm's unrelated daemon_restart_command.

The lint is untouched and un-weakened: git diff origin/main...245db8f0 -- scripts/ .test-pointer-allowlist.tsv .github/workflows/test-pointers.yml is empty. No deletion, no exclusion, no allowlist row. I reran it from a clean checkout of the head:

test-pointers: resolved 22088 Test: citation(s) (floor 200) — 0 dangling pointers — OK.

Two blocks the parser never examined — the right correction. canonical_bin_dir was reworded to lead with `canonical_bin_dir_from_*` rather than "the rule is covered by …", which moves it under the gate instead of around it. That is the fix that generalises: the parser stops at the first non-backticked token, so a prose-led block is invisible, and rewriting it to lead with the citation is the only way to make it verifiable. Good catch — it is the class of hole I did not look for last round.

The sign::default_bin_dir decision is correct, and it does not hide a gap. I verified the "two-line delegation" claim:

fn default_bin_dir() -> PathBuf {
    trusty_common::bin_resolve::canonical_bin_dir()
        .unwrap_or_else(|| PathBuf::from("/usr/local/bin"))
}

The lint scopes citations to the citing file's own crate, so a trusty-common test name cited from trusty-installer is a permanently unresolvable pointer — leading with it would trade a silent hole for a permanent red. The only behaviour local to this function is the /usr/local/bin last resort, reachable solely when canonical_bin_dir() returns None, which needs both an absent CARGO_HOME and a failing dirs::home_dir(). There is no seam for that here (the function takes no parameters and is #[cfg(target_os = "macos")]), and the None case of the rule itself is tested — canonical_bin_dir_from_is_none_without_either_input. So the untested surface is one unwrap_or_else literal, and the same shape sits untested at install.rs, install_one, and upgrade_one. Stating "no test in this crate, here is why, here is where the rule is proven" names the gap; it does not conceal it. Parameterising a two-line macOS-only helper to close it would be more churn than the risk warrants.

Finding 2 (MEDIUM) — the promise is withdrawn, not softened

No "may not" anywhere. upgrade.rs:9-21 states the condition, the consequence, and the verdict on its own report:

the daemon comes back on the NEW binary only when the plist's ProgramArguments[0] already points at the directory this upgrade wrote to … the bounce costs a downtime window and brings the SAME OLD BINARY back — while the report still says "upgraded to X; restarted". That is a false success report

Phase 4 is named as the prerequisite for the guarantee and Phase 3 as what removes the divergence. The changelog fragment carries the same declarative form ("this change stops the daemon being left un-bounced, it does not yet guarantee which binary it comes back on") — correct to qualify there too, since a user reading only the changelog would otherwise infer the guarantee. own_verb_control's which::which resolution is noted at lifecycle.rs:437-444 where it happens, stated as fact ("can therefore restart a different binary than the one it verified") with the deferral reasoned rather than asserted. No mechanism change, as instructed.

No regression across the fix

The change is doc comments plus a changelog fragment, and I confirmed that mechanically rather than by eye — every changed .rs line in e9a2a9a4..245db8f0 is a /// or //! line:

git diff e9a2a9a4..245db8f0 -- '*.rs' | grep '^[+-]' | grep -v '^[+-] *\(///\|//!\|//\)'
→ (no output)

No code moved while the comments around it were edited.

Gates at this head: cargo fmt --check && cargo test -p trusty-installer → EXIT=0, 562 passed, 0 failed, 4 ignored. Pointer lint green as quoted. The remaining gates were verified at e9a2a9a4 and cannot be affected by a doc-only diff.

Notes

The MEDIUM mechanism gap (the bounce re-execs whatever the plist names) is now documented in three places and stays with epic #4964 Phase 4 — that was the disposition, and it was honoured. Nothing installed, no daemon restarted, no file outside .claude/worktrees/critic-5011 touched.

🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
trusty-tools	Ready	Preview	Aug 11, 2026 5:08am