fix(daemon/launchd): prefer in-place kickstart on Restart; wait for label to disappear after bootout (#1833)

[chenhg5]

Closes #1833. Related to #35 (original 3 \xc3\x97 500 ms retry that only covered the idle-daemon case).

Root cause

When the daemon has agent sessions in flight, launchctl bootout returns BEFORE launchd has actually removed the job; the old daemon needs several seconds to tear down. The existing 3 \xc3\x97 500 ms bootstrap retry (\xe2\x89\x881 s total) then races against that teardown window and fails with Bootstrap failed: 5: Input/output error for every attempt. Restart() returns an error, the job has been removed from launchd, and KeepAlive cannot recover because the service is no longer loaded.

Reporter leishao provided a clear side-by-side timeline from cc-connect.log + unified log showing the daemon exiting 5 s after bootout, while the CLI finished bootout + 3 failed bootstraps in ~1 s. The previous 20+ successful restarts all happened while the daemon was idle, masking the race.

Fix

Two complementary paths in daemon/launchd.go::Restart():

1. Fast path \xe2\x80\x94 when the job is already loaded AND the canonical plist still exists on disk, use launchctl kickstart -k <target> (in-place). launchd sequences the SIGTERM/relaunch itself, so the bootout/bootstrap race cannot occur on this path.
2. Fallback path \xe2\x80\x94 bootout + a new waitLaunchdTargetsGone(30s) helper that polls loadedLaunchdTarget() every 200 ms until the label disappears, THEN run the existing 3 \xc3\x97 500 ms bootstrap retry as a safety net for genuine bootstrap races.

The plist-on-disk check is intentional: if a user has replaced the plist (binary path, env, etc.) they almost always want a full reinstall via Install(), not an in-place reload. The bootout path remains for the rare case where the job is not loaded or the plist has been removed between the daemon writing it and us restarting.

Tests added (darwin tag)

• TestRestartPrefersInPlaceKickstartWhenLoadedAndPlistPresent \xe2\x80\x94 verifies the in-place path is taken when conditions are met.
• TestRestartFallbackToBootoutWhenJobNotLoaded \xe2\x80\x94 verifies bootout + wait + bootstrap + kickstart when job is not loaded.
• TestRestartFallbackToBootoutWhenPlistMissing \xe2\x80\x94 verifies fallback when plist is removed (reinstall scenario).
• TestWaitLaunchdTargetsGoneReturnsImmediatelyWhenAlreadyGone \xe2\x80\x94 no wasted polls.
• TestWaitLaunchdTargetsGoneReturnsAfterJobDisappears \xe2\x80\x94 returns after the label actually disappears.
• TestWaitLaunchdTargetsGoneHonoursTimeout \xe2\x80\x94 soft timeout fires within ~5 poll intervals.

Local validation

• gofmt -l daemon/launchd.go daemon/launchd_test.go clean.
• GOOS=darwin go vet ./daemon/ clean.
• GOOS=darwin go test -c -o /tmp/daemon-darwin.test ./daemon/ builds cleanly (darwin binary; full execution requires macOS CI runner).
• go test ./daemon/ passes on linux tag (cross-platform surface unchanged).

Risk

• Low \xe2\x80\x94 changes are isolated to daemon/launchd.go and its test file. No public API change, no platform/agent code touched.
• The in-place path is opt-in by virtue of the loaded + plist-present check; on a freshly bootstrapped host (job not loaded) the fallback path runs exactly as before.
• The 3 \xc3\x97 500 ms bootstrap retry is preserved, so any pre-[Bug] macOS: daemon restart unloads the service for good when the daemon is busy (bootout/bootstrap race, follow-up to #35) #1833 success mode still works.

Files

• daemon/launchd.go \xe2\x80\x94 +84 / -4 lines (added Restart() fast path, waitLaunchdTargetsGone helper, 3 named constants for the timeouts).
• daemon/launchd_test.go \xe2\x80\x94 +320 lines (6 new tests).

[chenhg5]

QA Review — PR #1834

Thanks for the clean reproduction timeline from #1833 and the side-by-side cc-connect.log + unified log slice — that made the root cause unambiguous.

Conclusion: Approve ✅

This is mergeable. CI is 5/5 green on dee31ace4, mergeable_state is clean, the diff is 2 files / +404/-4 entirely inside daemon/launchd.go (//go:build darwin), and the test surface is genuine.

What looks good

• Race eliminated by construction, not by retry-til-it-works. The fast path lets launchd kickstart -k sequence SIGTERM/relaunch itself, so the bootout/agent-teardown race that #1833 documents cannot occur on this path. The fallback path then explicitly waits for the label to disappear via waitLaunchdTargetsGone(30s) before running the existing 3 × 500 ms bootstrap safety net — closing the same race on the bootout branch. This is the right shape; option A from the decision doc (just stretching retries) would have papered over the symptom.
• The plist-on-disk gate is principled. Gating fast-path on os.Stat(plistPath) instead of trying to diff contents is the correct trade-off — if a user has replaced the plist (binary path, env), they almost always want Install(), not an in-place reload. The bootout path remains as the fallback for that exact case, and the TestRestartFallbackToBootoutWhenPlistMissing test pins it.
• Constants are centralized and the rationale is in the comments. launchdRestartInPlaceTimeout = 60s (with the explicit "do not silently fall through to bootout" note), launchdRestartWaitPoll = 200ms, launchdRestartWaitDefault = 30s (comfortably above the reporter's 5 s observed teardown). The 30 s default is loud enough to surface a genuine launchd hang without flapping on slow teardowns.
• Soft timeout is the right behavior. When waitLaunchdTargetsGone hits the deadline it logs a WARN and returns, letting the existing 3 × 500 ms bootstrap retry handle the residual race. Operators get a log line; the user doesn't get a hard error on a transient slow launchd.
• Test coverage matches the branches exactly. 6 unit tests, all using a runLaunchctl stub so they don't depend on a real launchd: fast-path taken when loaded + plist present, fallback to bootout when not loaded, fallback to bootout when plist missing, wait helper returns immediately when already gone, wait helper returns after the label actually disappears, wait helper honours timeout. The fact that you can write TestRestartFallbackToBootoutWhenJobNotLoaded without launching macOS launchd is exactly the level of test isolation you want for this kind of code.
• Public API unchanged. Manager.Install / Start / Stop / Status signatures are untouched. Restart() only changes behavior on the narrow loaded && plist 在 condition; everywhere else the original behavior is preserved. Zero blast radius for any existing caller.
• No new dependencies. core / agent / platform / web are not touched.

🔵 Optional (P3)

• launchdRestartInPlaceTimeout is defined but currently unused. The comment promises "if the kickstart does not complete within this budget we surface the error rather than fall through to bootout," but the actual code path returns runLaunchctl's error directly without a context.WithTimeout wrap. Either wire it up via exec.CommandContext + a goroutine that reaps the process, or drop the constant (and the comment about not falling through). I'd lean toward wiring it up — the constant exists, the rationale is right, and the implementation is two lines — but it's a follow-up, not a blocker.

Testing / Risk

• ✅ CI: 5/5 green on dee31ace41f6961ed7fd7f85093b0885780975bd (lint / unit-test / smoke-test / regression-test / performance-test). unit-test runs on the macOS CI runner, so the 6 darwin-tagged tests are actually executed.
• ✅ Local: gofmt -l daemon/launchd.go daemon/launchd_test.go clean; GOOS=darwin go vet ./daemon/ clean; GOOS=darwin go test -c builds; go test ./daemon/ passes on Linux tag (cross-platform surface unchanged).
• ✅ Test files reviewed: daemon/launchd_test.go (+320 LOC) — 6 new tests, all stub-based and deterministic.
• 🟡 Remaining risk: live macOS daemon reproduction (#1833 path with sessions in flight) is only validated via the macOS CI unit tests. The reporter's original scenario (busy daemon, 5 s teardown) is well-modeled by the tests but a single manual smoke from a maintainer against a real agent session would be the cheapest confidence bump. Not blocking.

Next step

Maintainer: merge when convenient. Post-merge, the only thing I'd ask is a one-time manual smoke (cc-connect daemon restart while a session is active) and a glance at the release notes so the new behavior is documented. Solid bugfix.