Skip to content

MUL-7731 TAURUS-58 feat(cli): register daemon boot autostart on every platform - #8864

Open
zhangrui0517 wants to merge 8 commits into
multica-ai:mainfrom
zhangrui0517:agent/mika/f2e05cdd56fc
Open

zhangrui0517 wants to merge 8 commits into
multica-ai:mainfrom
zhangrui0517:agent/mika/f2e05cdd56fc

Conversation

@zhangrui0517

@zhangrui0517 zhangrui0517 commented Sep 26, 2026 •

Copy link
Copy Markdown

What does this PR do?

Opt-in boot autostart for the local agent runtime daemon, on all three release platforms. An explicit multica daemon autostart enable registers the profile's daemon with the OS so the machine brings it back after a reboot or re-login; daemon start never registers — it only prints a hint and refreshes what Multica itself created:

Platform Mechanism Where
Windows Per-user Run key HKCU\...\CurrentVersion\Run, value Multica / Multica (<profile>)
macOS launchd LaunchAgent ~/Library/LaunchAgents/ai.multica.daemon.plist
Linux systemd user unit ~/.config/systemd/user/multica-daemon.service; XDG autostart .desktop fallback without systemd

Design points, shaped by the review on this PR:

  • Opt-in only. multica daemon autostart enable|disable|status manage the registration. daemon start (background, --foreground, restart, and setup) prints Tip: run 'multica daemon autostart enable' … when nothing is registered and otherwise leaves the system alone. --no-autostart is gone — there is nothing to opt out of.
  • Refresh, never create, and only our own entries. While a registered entry exists, daemon start rewrites it in place so a moved executable (Homebrew upgrade, self-update) heals. Every file Multica writes carries an ownership marker — a # Managed by Multica … comment in units and .desktop files, a ManagedBy key in the plist (launchd ignores unknown keys), the value name on Windows. Entries without the marker (a hand-written unit such as the EnvironmentFile= one in [Bug]: Grok ACP handshake fails with auth method "grok.com" on systemd daemon (XAI_API_KEY not inherited) #7091) are never rewritten: refresh skips them and enable/disable refuse them by path. Refresh is additionally skipped entirely under an external supervisor (INVOCATION_ID set while /proc/self/cgroup does not name our unit).
  • Environment honesty. Only PATH is snapshotted into the entry (launchd/systemd login sessions would otherwise miss agent CLIs from Homebrew/nvm/user bins). enable states plainly that shell-exported variables (API keys, proxies) do not carry over and points at multica config set / user environment for them. loginctl enable-linger is never run automatically — enable prints the exact command as a note when linger is off.
  • systemd restart handoff fix. Under our own unit, spawning a binary-update successor and exiting 0 loses it: systemd sees a clean stop of Type=simple, cgroup cleanup kills the successor (Setsid escapes a session, not a cgroup), and Restart=on-failure never fires — the runtime would go offline until reboot after the first auto-update. The daemon now exits with a dedicated status (daemonSystemdHandoffExitStatus, 42) and the generated unit carries RestartForceExitStatus=42. Detection is INVOCATION_ID + our unit name in the cgroup path (new linux tests). Other supervisors (launchd's process-group kill, no supervisor) keep the existing spawn handoff.
  • Manager-owned daemons are untouched. Daemons with MULTICA_LAUNCHED_BY set (the Desktop app, which drives its daemon from its own app-start preference — it has no OS login item) are neither hinted, refreshed, nor registered.

Two supporting changes:

  • Windows console self-hide (internal/util/proc_windows.go): EnsureHiddenConsole hides a console when the daemon is the only process on it — otherwise a Run-key launch parks a visible console window on the desktop for the daemon's lifetime. A hosting terminal (attached shell) is never touched.
  • Test home isolation on Windows: os.UserHomeDir reads USERPROFILE, not HOME. TestMain redirects both to a scratch directory, and every HOME redirect in the package now goes through redirectTestHome (both vars), so suite runs can no longer write fixtures into a real ~/.multica — and read paths match write paths on Windows.

Related Issue

Closes TAURUS-58

(Multica workspace issue TAURUS-58 — 为所有运行时增加 multica daemon start 开机自启 — not a GitHub issue, so there is no # number.)

Type of Change

  • New feature (non-breaking change that adds functionality)
  • Bug fix (non-breaking change that fixes an issue) — systemd binary-update handoff
  • Documentation update
  • Tests (adding or improving test coverage)

Changes Made

  • server/cmd/multica/cmd_daemon_autostart.go — command group, hint/refresh hook (syncDaemonAutostart), ownership markers, platform-agnostic builders (plist/unit/desktop-entry, slug + quoting), status output, daemonSystemdHandoffExitStatus
  • server/cmd/multica/cmd_daemon_autostart_{windows,darwin,linux,other}.go — per-platform registration I/O, marker detection, external-supervisor gate (linux), read-only linger note
  • server/cmd/multica/cmd_daemon.go — no registration side effects, hint/refresh call sites, systemd handoff exit, help text
  • server/internal/util/proc_windows.go — hide a self-owned console
  • Tests: cmd_daemon_autostart_test.go (markers, opt-in contract, foreign-file refusal, PATH caveat), cmd_daemon_autostart_linux_test.go (cgroup unit detection + supervisor gates), seams in the existing daemon-start tests, redirectTestHome sweep
  • CLI_AND_DAEMON.md — "Boot autostart" section rewritten for the opt-in flow

How to Test

  1. multica daemon start — prints Tip: run 'multica daemon autostart enable' … while unregistered, and writes nothing to the OS. multica daemon autostart enable registers; status shows mechanism/location/command; disable removes.
  2. Inspect the registration: reg query HKCU\Software\Microsoft\Windows\CurrentVersion\Run (Windows), ~/Library/LaunchAgents/ai.multica.daemon*.plist (macOS), systemctl --user is-enabled multica-daemon.service (Linux). The files carry the # Managed by Multica / ManagedBy marker.
  3. Drop a marker-less file at one of those paths and rerun enable/daemon start — both refuse/skip it instead of overwriting.
  4. cd server && go test ./cmd/multica ./internal/util — run outside any .multica/daemon_task_context.json marker tree (the human-local-command tests walk up from the CWD). Linux CI also runs cmd_daemon_autostart_linux_test.go.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots (no UI change)
  • I have updated relevant documentation to reflect my changes (CLI_AND_DAEMON.md)
  • If I added a new runtime / coding tool / UI tab, I synced the change to landing copy and relevant docs (no new runtime/tool/UI tab)
  • If this PR touches Chinese product copy, I checked it against conventions.zh.mdx (no Chinese product copy changed)
  • I have considered and documented any risks above
  • I will address all reviewer comments before requesting merge

Verification on this Windows host: gofmt is clean against the committed (LF) content of every touched file; go vet is clean for windows/darwin/linux/freebsd; the targeted autostart/daemon suites pass. go test ./cmd/multica ./internal/util fails only 6 tests that reproduce identically on the base commit 344ffaf1b (three forward-slash path assertions, two unix-absolute-path assertions, one symlink-privilege case) — pre-existing Windows-host issues, unaffected by this PR and by the ubuntu CI job. gofmt was also applied to cmd_daemon_test.go and proc_windows.go per the review. The linux tests run in CI's ubuntu job (no systemd fixture needed, but they are linux-tagged).

Risks: registration is explicit, so nothing changes on machines that never run autostart enable; refresh only ever rewrites Multica-marked entries and skips external supervisors.

AI Disclosure

AI tool used: Multica Agent (Mika)

Prompt / approach: Workspace issue TAURUS-58 asked for boot autostart of multica daemon start for all runtimes. First pass shipped the default-on design; the review on this PR requested opt-in plus the systemd handoff fix, and this revision implements exactly that (plus the gofmt/wording/docs items).

@vercel

vercel Bot commented Sep 26, 2026

Copy link
Copy Markdown

@zhangrui0517 is attempting to deploy a commit to the IndexLabs Team on Vercel.

A member of the Team first needs to authorize it.

@Bohan-J Bohan-J changed the title TAURUS-58 feat(cli): register daemon boot autostart on every platform MUL-7731 TAURUS-58 feat(cli): register daemon boot autostart on every platform Sep 26, 2026

@Bohan-J Bohan-J left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. A CLI-started daemon not coming back after a reboot is a real gap, and the cross-platform groundwork here (per-profile naming, quoting, status output) is solid. Before this can land, we'd like two changes: registration should become opt-in, and the systemd restart handoff needs a fix. There are also a couple of smaller items.

1. Make registration opt-in instead of a side effect of daemon start

Right now daemon start, daemon start --foreground, daemon restart, and multica setup all register autostart implicitly. We'd rather not have a plain start write OS login entries:

  • Silent OS changes. Every start (including setup, which has no --no-autostart) writes a LaunchAgent, a systemd user unit, or a Run key. On Linux it also runs loginctl enable-linger, which is a system-level change: the user's processes are no longer reaped at logout.
  • Environment loss makes failures quieter. After a reboot the daemon only carries the PATH snapshot. API keys, HTTPS_PROXY, and similar values exported in the shell are gone. Today a reboot leaves the runtime visibly offline. With this PR it comes back online, claims tasks, and those tasks then fail, which is harder to diagnose.
  • --foreground is the supervised/debug mode. The docs describe it as the debug mode, and people run it under their own systemd units, Docker, or pm2. Registering from there means a debugging session enables boot autostart, and containers write autostart files or log a warning on every start. There is also a direct collision: some users already run a hand-written ~/.config/systemd/user/multica-daemon.service that calls multica daemon start --foreground (e.g. #7091, with an EnvironmentFile=). Because writeSystemdAutostart overwrites whenever content differs, the daemon started by that unit rewrites the user's unit on its next start and silently drops their EnvironmentFile=.
  • Dev environments. scripts/dev-env.sh (daemon start --profile <worktree>) and make daemon (daemon restart --profile local) would leave a login entry per worktree pointing at that worktree's server/bin/multica, and nothing removes it on destroy.

Suggested shape:

  • multica daemon autostart enable is the only thing that creates a registration.
  • daemon start only prints a one-line hint (e.g. Tip: run 'multica daemon autostart enable' to start this daemon at login) when nothing is registered. Drop --no-autostart since there is nothing to opt out of.
  • daemon start (background and foreground) may still refresh an existing registration to heal a moved executable path. It should only rewrite entries Multica itself created: add a marker (a comment line in the unit / .desktop, a plist key) and leave any file without it untouched. It should also skip refreshing entirely when running under an external supervisor it doesn't own (e.g. INVOCATION_ID set but the unit isn't ours).
  • Don't run loginctl enable-linger automatically. Keep the note that tells the user the exact command.
  • Make autostart enable say plainly that only PATH is carried over and that other shell-exported variables (API keys, proxies) are not. Point to where they should live instead.

2. systemd: auto-update / auto-reload takes the daemon offline

The restart handoff at the end of runDaemonForeground spawns the successor (exec.Command(restartBin, ...), Setsid), releases it, and exits 0. Under the generated unit (Type=simple, Restart=on-failure, default KillMode=control-group):

  1. The main PID exits 0, so systemd treats the service as stopped.
  2. Stopping the unit kills everything left in its cgroup, including the successor just spawned. Setsid does not escape a cgroup.
  3. Restart=on-failure does not fire on a clean exit.

Auto-reload is on by default for every CLI daemon, and auto-update is on by default for cloud. So on Linux, the first binary change after a boot-autostarted start (a GitHub auto-update, install.sh, a package upgrade) leaves the runtime offline until the next reboot, which is the exact problem this PR sets out to solve.

One way to fix it: when running under our own systemd unit (INVOCATION_ID set), don't spawn a successor. Exit with a dedicated status instead and add RestartForceExitStatus=<that status> to the unit, so systemd restarts it on the new binary. An in-place syscall.Exec on unix would also work. Either way, please add a test for the systemd branch.

As far as I can tell, launchd is fine here: the Setsid child leaves the job's process group, so launchd's default process-group kill doesn't reach it. That's worth a quick manual check too.

3. Smaller items

  • gofmt -l flags server/cmd/multica/cmd_daemon_test.go and server/internal/util/proc_windows.go.
  • The PR description says the Desktop app "has its own start-at-login preference". I couldn't find one in apps/desktop. Skipping registration for MULTICA_LAUNCHED_BY daemons is still correct, but the stated reason should be corrected (in the description and in the comment on shouldRegisterAutostart).
  • CLI_AND_DAEMON.md and the daemon start help text will need updating for the opt-in flow.

What I verified locally (macOS): the autostart, daemon-start, and task-context tests pass, and go vet is clean for linux/darwin/windows. The systemd behavior in item 2 comes from systemd's default semantics, not from a run on a real Linux host. I did not exercise the Windows Run-key path or the console-hiding change.

zhangrui0517 and others added 4 commits September 27, 2026 18:01
os.UserHomeDir reads USERPROFILE, not HOME, so every test that redirected
only HOME kept resolving ~/.multica against the real home: mkProfiles
stopped isolating at all, and a full-suite run wrote SaveCLIConfig
fixtures over a real default-profile config.json.

Redirect HOME and USERPROFILE process-wide in TestMain, and make mkProfiles
redirect both so per-test isolation holds on Windows too. On unix nothing
changes: USERPROFILE is unused there.

Co-authored-by: multica-agent <github@multica.ai>
multica daemon start now registers the profile's daemon to come back at
login/boot so a reboot no longer takes the runtime offline: an HKCU Run
key on Windows, a launchd LaunchAgent on macOS, and a systemd user unit
(or XDG autostart fallback without systemd) on Linux. New
'daemon autostart enable|disable|status' manages the registration;
--no-autostart opts a single start out, and daemons spawned by a manager
(MULTICA_LAUNCHED_BY, i.e. the Desktop app's own start-at-login pref) are
never registered.

The registered command runs 'daemon start --foreground' per profile and
snapshots the shell's PATH so login sessions keep finding agent CLIs.
EnsureHiddenConsole now hides a console that exists only for the daemon
itself — a Run-key launch would otherwise park a visible console window
for the daemon's whole lifetime.

Co-authored-by: multica-agent <github@multica.ai>
Registration now happens only through 'multica daemon autostart enable':
daemon start (and setup / restart / --foreground) merely print the enable
hint when nothing is registered and silently refresh an entry Multica
owns — marked by a comment line in a unit/.desktop file or a ManagedBy
key in a plist (the Run value name doubles as the marker on Windows).
Refresh skips anything without the marker and skips entirely under an
external systemd unit (INVOCATION_ID with a cgroup that is not ours), so
a hand-written unit like the one in multica-ai#7091 is never overwritten.
--no-autostart is gone (there is nothing to opt out of), loginctl
enable-linger is no longer run automatically (enable prints the command
instead), and 'autostart enable' now spells out that only PATH carries
into the login session while shell-exported variables do not.

Also fixes the binary-update restart handoff under our own systemd unit:
spawning a successor and exiting 0 lets systemd's cgroup cleanup kill it
while Restart=on-failure never fires, leaving the runtime offline after
the first auto-update. The daemon now exits with
daemonSystemdHandoffExitStatus (42) instead; the generated unit carries
RestartForceExitStatus for it, and linux tests cover the unit-ownership
detection behind both the handoff and the refresh gate.

Review items also addressed: gofmt applied to cmd_daemon_test.go and
proc_windows.go, the Desktop wording corrected to its app-start daemon
preference (it has no OS login item), and CLI_AND_DAEMON.md plus the
daemon start / autostart help text follow the opt-in flow.

Co-authored-by: multica-agent <github@multica.ai>
Every t.Setenv("HOME", ...) in this package now goes through
redirectTestHome, which sets both home environment variables. Production
resolves ~/.multica via os.UserHomeDir — USERPROFILE on Windows — so a
HOME-only redirect split the fixture's write path from the code's read
path: on Windows the fixture landed where the code never looked (or,
before TestMain's scratch home, in the real ~/.multica). CI on ubuntu is
unaffected; the Windows host suite stops depending on whatever happens
to be in the real profile directory.

Co-authored-by: multica-agent <github@multica.ai>
@zhangrui0517
zhangrui0517 force-pushed the agent/mika/f2e05cdd56fc branch from 6b27117 to b395424 Compare September 27, 2026 10:27
@zhangrui0517

Copy link
Copy Markdown
Author

@Bohan-J Thanks for the detailed review — everything you asked for is in the latest push (b395424ef), point by point:

1. Opt-in registration

  • multica daemon autostart enable is now the only thing that creates a registration. daemon start (background, --foreground, restart, and setup) prints Tip: run 'multica daemon autostart enable' to start this daemon at login when nothing is registered and writes nothing to the OS. --no-autostart is removed everywhere (start/restart flags, arg forwarding, help, docs).
  • The allowed refresh now only ever rewrites Multica-owned entries: a # Managed by Multica … comment in units and .desktop files, a ManagedBy key in the plist (launchd ignores unknown keys), the value name for the Run key. Files without the marker are never rewritten — refresh skips them, and enable/disable refuse them with the exact path (so a hand-written ~/.config/systemd/user/multica-daemon.service with EnvironmentFile=, e.g. [Bug]: Grok ACP handshake fails with auth method "grok.com" on systemd daemon (XAI_API_KEY not inherited) #7091, is safe; the platform writers also carry the same refusal as a backstop).
  • Refresh is additionally skipped entirely when running under an external supervisor: INVOCATION_ID set while /proc/self/cgroup does not name our unit (covered by the new linux tests).
  • loginctl enable-linger is no longer run — enable only reads linger state and prints the exact command as a note when it is off.
  • enable now prints, at registration time: only PATH is carried into the login session; shell-exported variables (API keys, HTTPS_PROXY, …) are not — persist daemon settings with multica config set and put the rest in user environment.

2. systemd restart handoff

Implemented the dedicated-status variant: under our own unit the foreground daemon logs the handoff and exits with daemonSystemdHandoffExitStatus (42) instead of spawning a successor; the generated unit carries RestartForceExitStatus=42. Detection is INVOCATION_ID and the unit name in /proc/self/cgroup, so a daemon started by the user's own differently-named unit keeps the portable spawn path — same for launchd and no supervisor, per your analysis. Tests: cmd_daemon_autostart_linux_test.go covers the cgroup matcher (own/foreign/named-profile/unreadable) and both env gates; the unit-content test pins RestartForceExitStatus to the same constant the daemon exits with.

3. Smaller items

  • gofmt applied to cmd_daemon_test.go and proc_windows.go — verified clean against the committed (LF) content of every touched file.
  • Desktop wording corrected: it is the app's app-start daemon preference, not a start-at-login preference — fixed in the PR description and in the shouldManageAutostart comment (the exclusion itself stays).
  • CLI_AND_DAEMON.md and the daemon start / daemon autostart help text now describe the opt-in flow.

Also folded in while verifying on this Windows host: every HOME redirect in the package now goes through a redirectTestHome helper that sets USERPROFILE too (os.UserHomeDir reads USERPROFILE on Windows), so suite runs neither split fixture/read paths nor write into a real ~/.multica. CI-visible behavior on ubuntu is unchanged.

Verification: go vet clean for windows/darwin/linux/freebsd; targeted autostart/daemon suites green; gofmt clean on committed content. Full go test ./cmd/multica ./internal/util on this Windows host fails only 6 tests that reproduce identically on the base commit 344ffaf1b (three forward-slash path assertions, two unix-absolute-path assertions, one symlink-privilege case) — pre-existing Windows-host issues. The linux tests are linux-tagged and will run in the ubuntu job; -race needs a C toolchain this host does not have.

@Bohan-J Bohan-J left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast turnaround. Every point from the last round is addressed. The opt-in flow, the ownership markers with refusal, the supervisor gate, the read-only linger note, and the systemd handoff via RestartForceExitStatus all look right to me. One issue remains, and it comes from the new handoff.

Linux + Homebrew: the systemd handoff restarts a deleted binary

install.sh installs through Homebrew whenever brew is on the machine, and that includes Linux (knownBrewPrefixes lists /home/linuxbrew/.linuxbrew). For such an install:

  1. autostart enable writes ExecStart= from autostartSpecFor → daemonExecutable() → os.Executable() (server/cmd/multica/cmd_daemon_autostart.go:431). On Linux that resolves /proc/self/exe, so the unit pins the versioned keg path, e.g. /home/linuxbrew/.linuxbrew/Cellar/multica/0.4.33/bin/multica.
  2. Auto-update on a brew install runs brew upgrade multica-ai/tap/multica. By default this also cleans up the old keg, so …/Cellar/multica/0.4.33/ is removed.
  3. The daemon, running under our unit, takes the new branch at server/cmd/multica/cmd_daemon.go:1137 and exits 42.
  4. systemd restarts ExecStart, which is the deleted path, and fails with 203/EXEC. After StartLimitBurst attempts it gives up. The runtime stays offline until someone starts it by hand.

The spawn handoff this branch replaces did not have this problem. It restarts restartBin, and for brew installs restartTargetBinary() (server/internal/daemon/daemon.go:5302) deliberately returns the stable <brew prefix>/bin/multica, not the keg path. The systemd branch drops restartBin and relies on whatever the unit recorded.

Suggested fix: resolve the registered executable the same way restartTargetBinary() does. For brew installs that means <prefix>/bin/multica, and the resolved path otherwise. Put this in one shared helper so the two can't drift apart. It also covers a smaller macOS case: if the daemon isn't running when a brew upgrade cleans up the old keg, the LaunchAgent would point at a deleted path at next login, because the refresh only happens when a daemon starts. A unit test that feeds a brew-style keg path into autostartSpecFor and asserts the stable prefix path would pin this down.

A simpler alternative is to refresh the owned unit with restartBin and run systemctl --user daemon-reload right before exiting 42. It fixes the Linux case only, so I'd prefer the shared-resolver approach.

Verified locally: the autostart suites pass, go vet is clean for linux/darwin/windows, gofmt is clean, and CI backend-tests (ubuntu, including the new linux-tagged tests) and windows-execenv are green. The failure sequence above comes from reading the code alongside Homebrew's default cleanup and systemd's restart semantics; I haven't reproduced it on a Linuxbrew host.

Once this is in, I'm happy to approve.

… restart target

The exit-42 systemd handoff restarts whatever ExecStart recorded, and
autostartSpecFor recorded os.Executable() verbatim — on brew installs
(Homebrew on Linux too, reached through install.sh) that is the versioned
keg path, which `brew upgrade` deletes when it cleans the old keg:
systemd then fails ExecStart with 203/EXEC and gives up after
StartLimitBurst, leaving the runtime offline until someone starts it by
hand; a LaunchAgent would point at a missing file at the next login.

Fix per review: both consumers now resolve through one shared helper,
cli.StableExecutablePath, with identical detection order — the path's own
known-Cellar shape first, then `brew --prefix` — so the two cannot drift.
The daemon's restartTargetBinary keeps its seam-based per-process cache
but delegates the mapping; autostartSpecFor runs
cli.StableSelfExecutable before recording. cli.IsBrewInstall shares the
same detection core.

Tests pin the keg -> <prefix>/bin/multica mapping at the helper level for
all three known prefixes, and end to end through autostartSpecFor feeding
a brew keg path, as requested in the review.

Co-authored-by: multica-agent <github@multica.ai>
@zhangrui0517

Copy link
Copy Markdown
Author

@Bohan-J Fixed per your preferred approach — pushed as 4737e6c7b.

Shared resolver (one helper, no drift):

  • cli.StableExecutablePath(path, brewPrefix, brewInstall) is now the single mapping both consumers use: brew installs → <prefix>/bin/multica, non-brew → symlink-resolved path, unresolved brew prefix → path unchanged (caller warns).
  • cli.StableSelfExecutable(path) is the one-stop form for callers without a brew-fact cache, and cli.IsBrewInstall shares the same detection core (brewInstallForPath).
  • Daemon.restartTargetBinary keeps its seam-based per-process cache (so the daemon's existing stubs and the per-tick brew --prefix avoidance are unchanged) but delegates the mapping to the shared helper — it no longer has its own copy of the join.
  • Detection order is identical everywhere and documented: the path's own known-Cellar shape first, then brew --prefix — so a keg path always resolves to that keg's prefix and the restart target and a recorded ExecStart can't disagree.

Call sites:

  • autostartSpecFor now runs exe through cli.StableSelfExecutable before building the spec, so the systemd unit / LaunchAgent records <prefix>/bin/multica and the exit-42 handoff restarts a path brew upgrade never deletes. This also covers the macOS case you noted (keg cleaned while the daemon isn't running — the LaunchAgent already points at the stable path).
  • Per your test suggestion: TestAutostartSpecForUsesStableBrewPath feeds a brew-style keg path (/opt/homebrew/Cellar/multica/0.4.33/bin/multica) into autostartSpecFor and asserts /opt/homebrew/bin/multica. Helper-level tests in internal/cli/stable_exe_test.go pin the mapping for all three known prefixes plus the non-brew symlink/passthrough cases; they resolve via the offline known-Cellar match, so they are deterministic with or without brew on the test host.

Verification: gofmt clean (checked against LF-normalized content), go vet clean for windows/darwin/linux/freebsd. go test ./internal/cli ./internal/daemon on this Windows host now fails exactly the same set as the base commit (72 = 72, zero delta — all pre-existing Windows-host issues), and ./cmd/multica ./internal/util in a clean worktree shows only the same 6 pre-existing failures already reproduced on base. Your two TestTriggerRestart_Brew* tests pass again — my first cut used plain string concatenation instead of filepath.Join, which changed the mapping on Windows; reverted to Join (identical on unix where brew exists) with the new tests' expectations written in Join form.

Ready for another look.

@Bohan-J Bohan-J left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The stable brew path fix in 4737e6c7b looks right: registration and the restart target now share one resolver, and the new tests pin it. There is one more gap, this time in the ownership guard. On Linux, a file Multica didn't create can still be deleted, which breaks the "files without the marker are never touched" contract.

Linux: unmarked files can still be deleted

1. The guard keys off Enabled, and on Linux Enabled only means "linked in default.target.wants".

refuseUnmanagedAutostart (server/cmd/multica/cmd_daemon_autostart.go:202) refuses only when Enabled && !Managed. For systemd, platformReadAutostart sets Enabled from systemdUnitLinked (cmd_daemon_autostart_linux.go:92, :294), and that function only checks default.target.wants/. So a hand-written multica-daemon.service gets past the guard whenever it is:

  • present but not enabled, or
  • enabled under a different target, e.g. WantedBy=graphical-session.target, which is common for desktop-session user services.

In both cases, status reports disabled with no "not created by Multica" note. autostart disable then passes the guard and reaches platformRemoveAutostart (:226), which removes the unit as soon as it stats successfully, without re-checking the marker.

2. The XDG cleanup never checks the marker, and it also runs on every daemon start.

removeXdgAutostartFile (:273) deletes multica-daemon.desktop unconditionally. It is called from writeSystemdAutostart (:154) and platformRemoveAutostart (:259). Once a Multica-managed unit exists, the read/guard path only looks at the unit, so a hand-written .desktop with the same name gets deleted by autostart enable, by autostart disable, and by the silent refresh on a plain multica daemon start.

Reproduction

I ran this on 4737e6c7b with a throwaway linux-tagged test: XDG_CONFIG_HOME pointed at a temp dir, and a fake systemctl on PATH that creates/removes the default.target.wants symlink on enable/disable. The test was cross-compiled with CGO_ENABLED=0 GOOS=linux go test -c and run in alpine:3.21. The tests called runDaemonAutostartEnable / runDaemonAutostartDisable / syncDaemonAutostartDefault directly:

Setup Action Result
Unmarked multica-daemon.service, no wants link autostart disable returns nil, unit deleted
Unmarked unit linked in graphical-session.target.wants autostart status → autostart disable status: enabled=false managed=false; unit deleted
Unmarked unit linked in default.target.wants (control) autostart disable refused, unit kept ✅
Managed unit registered, then unmarked multica-daemon.desktop added daemon start refresh .desktop deleted
same autostart enable / autostart disable .desktop deleted in both

These all need a hand-written file at exactly our name, so they are unlikely. But when it happens, the result is the silent loss of a user's configuration, which is exactly what the marker was introduced to prevent.

Suggested fix

  • Model "a file exists at our path" separately from "enabled". The guard should refuse whenever a file exists without the marker, regardless of link state. status should also surface the "not created by Multica" note in that case.
  • In platformRemoveAutostart, re-read and check the marker right before each os.Remove, as a backstop that mirrors the writers.
  • Make removeXdgAutostartFile delete only a marked .desktop and leave an unmarked one in place.
  • Add linux tests for the unlinked unmarked unit, the unmarked unit enabled under another target, and a managed unit coexisting with an unmarked .desktop across enable, refresh, and disable.

Everything else from the previous rounds still holds, and CI is green on 4737e6c7b. Once this is in, I'm happy to approve.

Review round 3: on Linux Enabled only means "linked in
default.target.wants", so the enable/disable guard (Enabled && !Managed)
missed a hand-written unit at our path that was unlinked — or linked
under another target such as graphical-session.target — and disable
deleted it; separately, the XDG sweeper deleted an unmarked
multica-daemon.desktop outright from enable, disable, and the silent
daemon start refresh.

- autostartState gains Present ("a file/value exists at our path");
  refuseUnmanagedAutostart and the status note key off Present, while the
  enabled/disabled verdict still keys off Enabled.
- platformRemoveAutostart re-checks the marker on the unit BEFORE
  systemctl disable (refusing must not unlink the user's unit as a side
  effect) and refuses to delete an unmarked file; removeXdgAutostartFile
  removes only marked files, which covers all three of its call sites
  (enable's stale sweep, the daemon start refresh, disable).
- sync no longer hints enable at a foreign unlinked file, and never
  heals an owned-but-unlinked entry — refresh must not re-enable what the
  user manually disabled — it hints instead.
- linux tests cover unlinked / other-target-linked / default-linked
  unmarked units on the read side, the removal backstop, a managed unit
  coexisting with an unmarked .desktop, and the mark-checked XDG
  sweeper; shared guard/status/sync tests move to the presence contract.

Co-authored-by: multica-agent <github@multica.ai>
@zhangrui0517

Copy link
Copy Markdown
Author

@Bohan-J Fixed per your four suggestions — pushed as 1de086813.

1. Presence modeled separately from enablement

  • autostartState gains Present ("a registration file/value exists at our path"). On Linux a unit file sets Present=true while Enabled keeps meaning exactly "linked in default.target.wants"; on Windows/macOS presence and enablement coincide and both are set.
  • refuseUnmanagedAutostart now keys off Present && !Managed, so the unlinked unmarked unit and the graphical-session.target-linked one are both refused — no link-state dependency.
  • status flags not created by Multica off Present too, so the verdict line can honestly read disabled while the note still says whose file it is (pinned by new tests for both the unlinked and other-target shapes).

2. Removal backstop mirroring the writers

  • platformRemoveAutostart re-reads the unit and checks the marker before systemctl disable — refusing after the disable would already have unlinked the user's unit — and never reaches os.Remove for an unmarked file. Backstop tested directly: unmarked unit → errAutostartUnmanaged, changed=false, file intact.

3. removeXdgAutostartFile only removes marked files

It now reads the file first and leaves anything without the marker in place (unreadable counts as "not ours" — never delete what we can't positively identify). Since enable's stale sweep, the daemon start refresh, and disable all funnel through this one function, that covers all three of your call sites with one check. Tested both directions (unmarked survives, marked removed), plus your coexistence scenario end to end at the platform level: managed unit + unmarked .desktop → disable removes the unit and leaves the .desktop (TestPlatformRemoveAutostartKeepsUnmarkedXdgAlongsideManagedUnit).

4. Sync behavior for the new states (one judgment call beyond the list — please sanity-check):

  • Foreign and present → silent (no rewrite, no hint): hinting enable at it would only lead to enable's own refusal.
  • Owned but unlinked (e.g. the user ran systemctl disable) → hint, never a silent heal: the refresh must not re-link an entry the user deliberately turned off. Pinned by tests both ways.

Tests added (linux-tagged, run in the ubuntu job): unlinked unmarked unit, unmarked unit linked under graphical-session.target.wants, unmarked unit linked under default.target.wants (the read-side control), the removal backstop, managed-unit + unmarked-.desktop coexistence, and the mark-checked XDG sweeper. Shared guard/status/sync tests were moved to the presence contract, including disable-refuses-unlinked-foreign and status-flags-unlinked-foreign.

Verification on this Windows host: gofmt clean on LF-normalized content; go vet clean for windows/darwin/linux/freebsd; the linux test binary cross-compiles and links (GOOS=linux go test -c); targeted suites pass; full ./cmd/multica shows only the same 6 pre-existing failures reproduced on base, and ./internal/cli ./internal/daemon are byte-identical to base (72 = 72, zero delta). The linux tests themselves execute in CI's ubuntu job.

Ready for another look.

@Bohan-J Bohan-J left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ownership fixes in 1de086813 address the previous review. One remaining interaction needs a fix before merging.

[P2] Disabling autostart can take a running Linux daemon offline at its next update

This affects a daemon already running under the generated systemd user unit:

  1. The user runs multica daemon autostart disable, intending to prevent future login starts while continuing the current session.
  2. platformRemoveAutostart disables the unit, then deletes its file and runs daemon-reload. The current daemon remains running, but systemd loses the unit's restart configuration.
  3. On the next auto-update or binary reload, the handoff branch still exits 42: its supervisor detection checks INVOCATION_ID and the cgroup name, which have not changed.
  4. systemd no longer restarts it. The runtime stays offline until someone manually starts the daemon; tasks can remain queued if no other eligible runtime is available.

A real systemd user-session reproduction on this commit used the PR-built CLI for autostart disable and a fixture process to simulate the exit-42 handoff:

Disable operation Immediately afterward After exit 42
PR's autostart disable Process still active; LoadState=not-found, Restart=no MainPID=0, NRestarts=0, ActiveState=failed
Control: systemctl --user disable, retaining the unit, then daemon-reload Process active; restart policy retained New PID, NRestarts=1, ActiveState=active

This isolates the disable/handoff interaction; it does not exercise a full CLI download-and-upgrade cycle.

Please preserve the running service's restart configuration when disabling boot autostart. Removing the enablement link while retaining the managed unit is one approach supported by the control result. Add regression coverage for running service → disable autostart → binary-update handoff: the daemon should recover automatically in the current session, autostart should remain disabled, and the restarted daemon must not silently re-enable it.

…tostart

Review round 4 (P2): on Linux, `autostart disable` deleted the managed
unit file after unlinking it. The running daemon kept going, but systemd
lost the unit — LoadState=not-found, Restart=no — so the next
binary-update handoff (exit 42) exited into a service that could no
longer restart it and the runtime stayed offline until a manual start.

Disable now removes the enablement only: unlink every wants entry
(systemctl --user disable, with a glob fallback for a missing user bus)
and KEEP the marked unit file, so the supervised process retains its
restart policy while nothing starts at the next login. Re-enabling
re-links the same file. A second disable is a no-op again (change is
keyed off links actually present, in any target). The macOS equivalent
is fixed in the same spirit: the plist is deleted WITHOUT launchctl
bootout, which used to terminate the daemon launchd was supervising —
disable means "don't start at the next login", not "kill this session".

enable/disable now read the state once and share it with the ownership
guard; enable's wording keys off the pre-state, because re-linking a
kept unit often changes no file content and "already enabled" would
describe the state the user just left. The daemon-start hint now fires
only when nothing is registered at all — never at a disabled-or-foreign
file the user chose or owns — and the refresh still never re-links.

Regression coverage for running service → disable → handoff:
TestDisableKeepsRunningServiceRecoverable asserts the unit file survives
with RestartForceExitStatus intact, no wants link remains, the handoff
still detects our own unit (exit-42 path), a subsequent sync does not
re-enable, and a second disable reports no change. Docs note the
keep-the-file semantics on both Linux and macOS.

Co-authored-by: multica-agent <github@multica.ai>
@zhangrui0517

Copy link
Copy Markdown
Author

@Bohan-J P2 fixed — pushed as 81d239ae3.

Linux disable now removes the enablement and keeps the unit (your control condition):

  • platformRemoveAutostart unlinks every wants entry (systemctl --user disable, with a glob-based fallback across *.wants when there is no user bus — so a graphical-session.target link is cleared too) and no longer deletes the managed unit file. The supervised process keeps Restart= / RestartForceExitStatus=, so the exit-42 handoff still has a policy to restart under; nothing starts at the next login because nothing is linked. Re-enable simply re-links the same file.
  • Disable stays idempotent: change is now keyed off links actually present (any target), so a second disable reports "not enabled".
  • The unit file that remains is marker-owned and unlinked — enable/disable/refresh all treat it as ours-but-disabled, exactly the state round 4 already models.

Same-class macOS fix (flagging explicitly since your table was Linux-only): platformRemoveAutostart on darwin deleted the plist after launchctl bootout, and bootout terminates the very process launchd was supervising — disable was killing the running daemon immediately, a harsher version of the same bug. The plist is now deleted without bootout: launchd keeps the running process, and next login starts nothing (plist presence is the enablement there, so deleting it is still a true disable). Windows is unaffected — deleting the Run value does not touch the running process.

enable/disable now read state once and share it with the guard; enable's wording keys off the pre-state, because re-linking a kept unit often changes no file content — already enabled would describe the state the user just left.

Hint rule simplified accordingly: the daemon start hint fires only when nothing is registered at all — never at a disabled-or-foreign file (someone who just ran autostart disable should not be nudged to enable). The refresh still never re-links.

Regression coverage for your scenario — TestDisableKeepsRunningServiceRecoverable (linux, CI): marked+linked unit → platformRemoveAutostart → asserts (1) the unit file survives with RestartForceExitStatus= intact (your control's restart policy), (2) no wants link remains (autostart disabled), (3) the handoff still detects our own unit for this process (exit-42 branch still taken), (4) a following sync does not re-enable or touch the file, (5) a second disable reports no change. Shared tests updated: enable says enabled when re-linking a kept unit, and the hint never fires for present-but-disabled entries. CLI_AND_DAEMON.md documents the keep-the-file semantics on both platforms.

Verification: gofmt clean (LF-normalized), go vet windows/darwin/linux/freebsd green, linux test binary compiles and links, targeted suites pass, full ./cmd/multica shows only the same 6 pre-existing base failures. internal/cli and internal/daemon are untouched in this round. The linux test executes in CI's ubuntu job.

Ready for another look.

…ontract

CI caught the round-4 test asserting the pre-round-5 behavior: it wrote a
marked unit WITHOUT a wants link and expected platformRemoveAutostart to
delete it. Disable now keeps the marked unit (unlinking its enablement)
so a running service retains its restart policy — so the test links the
unit (giving disable real work, changed=true), asserts the unit survives
unlinked, and keeps its original point: the hand-written .desktop beside
it is never touched.

Co-authored-by: multica-agent <github@multica.ai>
@zhangrui0517

Copy link
Copy Markdown
Author

CI was red on 81d239ae3 — one real failure behind two red jobs, now fixed and pushed as 00ddac8e1:

  • backend-tests failed on TestPlatformRemoveAutostartKeepsUnmarkedXdgAlongsideManagedUnit: the round-4 coexistence test still asserted the pre-round-5 contract (marked unit deleted, changed=true for a unit written without a wants link). Round 5 deliberately keeps the marked unit and only counts a change when enablement links are removed — so an unlinked fixture correctly reported changed=false.
  • backend was just the aggregate gate echoing that failure (backend-tests: expected success, got failure); build/vet/migrations had passed.

The test now links the unit (giving disable real work), asserts the unit survives unlinked (restart policy retained), and keeps its original point — the hand-written .desktop beside it is never touched. Locally: gofmt clean, go vet green, linux test binary compiles+links, targeted suites pass. New CI run is starting on 00ddac8e1.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants