Fix: Upgrades fail with launchctl EIO (bootout race) - #880
Merged
Conversation
v0.7.0-alpha.6 cannot upgrade an existing install. Reported from a real run:
Setting up the launchd user agent...
abctl: launchctl bootstrap failed: exit status 5: Bootstrap failed: 5: Input/output error
error: could not set up the service (exit 1).
Cortex is NOT running.
`launchctl bootout` returns before the job has left the domain, and bootstrapping into
that window fails with EIO. Reproduced directly against a throwaway label: bootout
followed immediately by bootstrap gives `rc=5 Input/output error` when the job is slow
to die, and succeeds when it dies instantly.
Slow to die is us, by design: bootout SIGTERMs the supervisor, which forwards to the
proxy and waits out its 15s graceful shutdown before insisting. So the window is wide
enough to lose reliably — which is why this hits every existing user and no test.
Every end-to-end run I did either started from nothing or ran `service uninstall`
first, so none of them exercised install-over-a-running-service. That is the upgrade
path, and it is the only path an existing user takes. The gap was in what I chose to
test, not in the testing.
loadService now polls until the label is actually gone (30s cap, longer than the
supervisor's own 20s teardown) and reports a clear error if it never does, rather than
handing back launchd's EIO. Bootstrap additionally retries up to three times on EIO, to
close the race inside the window as well as the window itself. Darwin only; systemd's
enable/disable does not have this failure mode and its branch is untouched.
Verified: reinstalling over a running service now succeeds twice in a row, healthy both
times, on a label-isolated build.
The test drives real launchctl and skips when launchd will not start the fixture — it
first passed in 0.07s against a job that had never run, green and proving nothing, so it
now checks for `state = running` and skips loudly instead. It also asserted that the
wait took at least a second; that was flaky, because launchd sometimes tears a job down
in milliseconds, so a correct implementation failed. Removed. What it pins is the
post-condition: the label is gone, and the bootstrap that used to hit EIO succeeds.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ke a hang Six findings, all valid. The first two are the same criticism I earned last round. **Nothing covered the call site.** Verified by mutation: deleting the waitBootedOut call from loadService left the whole suite green — the second time in this PR series that a test asserted a helper's content while the wiring that mattered went unpinned. TestBootoutWaitIsWiredIn now reads the source and requires bootout → wait → bootstrap in that order, using the idiom TestStopIsDurable already established, and it runs on Linux CI unlike the launchd test. Re-mutated with a form that still compiles: it fails. **The launchd test had four skip paths**, so it could report success having executed no assertion — on the machine a release is built from. ABCTL_LAUNCHD_TESTS=required turns every skip into a failure, so a release check can prove the race was exercised rather than hope it was. This bug shipped because a path was never exercised. **Up to 30s of silent polling** sat immediately after "Setting up the launchd user agent...", indistinguishable from a hang, for exactly the people who just hit the EIO failure. A progress writer is plumbed through loadService and controlService; one line after the first second, so a fast teardown stays silent. Testing that needed a change: there is no way to hold a real launchd label half-torn-down on demand, so the first attempt could only skip. The polling is now injectable (waitGone takes the predicate), and the progress behaviour is covered deterministically — silent fast path, announce-then-confirm slow path, timeout, and a nil writer. **A refused bootout was reported as "still shutting down".** Wrong cause, and it told the user to try again when retrying cannot help. bootout's output is now kept and consulted only if the label is still present, so "could not remove" and "still shutting down" are distinguishable. Its error alone is not a signal — it also fails when nothing was loaded, the common case. **The retry comment was only true for the first attempt.** An attempt that registers the label and then fails leaves the next returning "File exists" rather than EIO. Each retry now re-checks the domain, making the claim true as written. **Dropped the unnecessary var hoisting** in the bootstrap loop. One thing the review's own suggestion cost: refactoring to a `target` variable broke TestStopIsDurable, which matched the exact former literal. That is the standing price of reading source instead of driving code — the assertion is looser now, and says so. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
mrsabath
approved these changes
Sep 4, 2026
mrsabath
left a comment
mrsabath
left a comment
Contributor
There was a problem hiding this comment.
Correct, well-diagnosed, and well-tested fix for the bootout/bootstrap EIO race. I verified the substance:
- Ordering is right —
enable → bootout → waitBootedOut → bootstrap(retry) → kickstart, with the wait placed exactly between bootout and bootstrap;timeis imported; the systemd path is correctly untouched (no equivalent async-teardown failure mode). - Fresh-install fast path preserved — booting out an absent label makes
launchctl printfail immediately, sowaitBootedOutreturns instantly rather than burning 30s on someone with nothing to tear down. - The two mechanisms compose —
waitBootedOuttreats any non-zeroprintas "gone" (a mild over-approximation), but the bootstrap EIO-retry is precisely the compensating control for any residual window. Belt-and-suspenders, correct by design. - The test earns its keep — it drives real launchctl and specifically guards against the vacuous pass that hid this originally: it requires
state = runningbefore exercising the race and skips loudly otherwise, and it drops a flaky timing assertion in favour of the post-condition that matters (label gone, and the bootstrap that used to hit EIO now succeeds). Honest, too, that no prior test exercised install-over-running-service — which is exactly the gap this closes.
No blocking issues and nothing worth an inline nit. All 21 CI checks green, one signed-off commit. Worth expediting per your note — alpha.6 is broken for the upgrade path, the only path an existing user takes.
Reviewed: the full diff plus cmd_service_platform.go at the head SHA. Did not run the launchctl test (macOS-gated, registers a real agent) — the PR reports reinstall-over-running succeeds twice, healthy both times, on a label-isolated build.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v0.7.0-alpha.6 cannot upgrade an existing install. From a real run:
launchctl bootoutreturns before the job leaves the domain, and bootstrapping into that window fails with EIO. Reproduced directly against a throwaway label: bootout followed immediately by bootstrap givesrc=5 Input/output errorwhen the job is slow to die, and succeeds when it dies instantly.Slow to die is us, by design — bootout SIGTERMs the supervisor, which forwards to the proxy and waits out its 15s graceful shutdown before insisting. The window is wide enough to lose reliably, which is why this hits every existing user.
Why no test caught it
Every end-to-end run either started from nothing or ran
service uninstallfirst. None exercised install-over-a-running-service — the upgrade path, and the only path an existing user takes.Fix
loadServicepolls until the label is actually gone (30s cap, longer than the supervisor own 20s teardown) and reports a clear error rather than handing back launchd EIO. Bootstrap also retries up to 3x on EIO. Darwin only; systemd has no such failure mode.Verified: reinstall over a running service succeeds twice in a row, healthy both times, on a label-isolated build.
Note on the test
It drives real launchctl. It first passed in 0.07s against a job launchd had never started — green, proving nothing — so it now checks for
state = runningand skips loudly. It also asserted the wait took >=1s; that was flaky (launchd sometimes tears down in milliseconds) and is removed. What it pins is the post-condition: label gone, and the bootstrap that used to hit EIO succeeds.This should ship as alpha.7 — alpha.6 is broken for upgrades.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com