fix: remove ShipIt's launchd job once installation completes - #331
MarshallOfSound wants to merge 1 commit into
Conversation
A job submitted with SMJobSubmit is a registration, not a one-shot: after ShipIt exits, the job stays in the launchd domain as "not running" until the login session ends. macOS 27 shows a Dock tile for any app with a registered background job, so an updated-and-quit app looks like it is still running (#21). ShipIt now removes its own job on the terminal success paths (install completed, or cancelled because the app relaunched), right before exit(0). Failure exits keep the registration so KeepAlive can respawn ShipIt to retry the install. The removal passes a NULL authorization: by that point the installer has replaced the bundle containing the running ShipIt binary, so authd can no longer validate its code signature and AuthorizationCreate fails with errAuthorizationDenied even for root. SMJobRemove with NULL authorization authorizes on the caller instead — root may modify the system domain and any caller its own user domain — which never prompts and works with the binary already gone. SIGTERM is ignored during removal because launchd terminates a job it is removing, and wait=false avoids deadlocking on our own exit.
VerteDinde
left a comment
VerteDinde
left a comment
There was a problem hiding this comment.
Read through this against the launcher/updater side looking for ways the self-removal could bite (KeepAlive/Mach-demand respawn after wait=false, pgroup kill of the relaunched app, wrong domain, exit-0 paths that skip it, the app resubmitting concurrently) and it holds up. A few small things inline.
One heads-up for the Electron roll rather than this repo: Electron's GN ShipIt executable target only links AppKit/Foundation/IOKit/Security, so it'll need ServiceManagement.framework added when DEPS picks this up.
| CFErrorRef cfError = NULL; | ||
| if (!SMJobRemove(domain, (__bridge CFStringRef)jobLabel, NULL, false, &cfError)) { | ||
| NSError *error = CFBridgingRelease(cfError); | ||
| NSLog(@"Could not remove ShipIt launchd job %@: %@", jobLabel, error); |
There was a problem hiding this comment.
nit: worth swallowing kSMErrorJobNotFound here like SQRLShipItLauncher and the test fixture do — it happens legitimately if ShipIt is run outside launchd, or if a just-relaunched app's launchPrivileged: removes/resubmits the label before we get here.
| // caller itself — root may modify the system domain and any caller its | ||
| // own user domain — which neither consults authd nor can present a | ||
| // prompt, and works with the binary already gone. | ||
| CFStringRef domain = (geteuid() == 0 ? kSMDomainSystemLaunchd : kSMDomainUserLaunchd); |
There was a problem hiding this comment.
q: this only clears the domain this run lives in. Someone who previously did an admin-prompted (system-domain) update keeps that stale job indefinitely and later unprivileged runs can't remove it, so the Dock tile could persist for them. Intentional / worth a note?
| // launchd terminates a running job as part of removing it. Ignore | ||
| // SIGTERM so we still exit through our own exit() call with the | ||
| // intended status rather than dying by signal mid-cleanup. | ||
| signal(SIGTERM, SIG_IGN); |
There was a problem hiding this comment.
q: if the install lands during logout/restart (the on-demand-only case) and the SMJobRemove IPC stalls, we now ignore launchd's TERM and hold the session for ExitTimeOut until KILL. Probably fine, but an alarm() here would bound it.
| // causes macOS 27 to show a Dock tile for the updated app after quit. | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| expect(CFBridgingRelease(SMJobCopyDictionary(kSMDomainUserLaunchd, (__bridge CFStringRef)self.shipItDirectoryManager.applicationIdentifier))).withTimeout(SQRLLongTimeout).toEventually(beNil()); |
There was a problem hiding this comment.
nit: in the suite ShipIt runs from the test host's Squirrel.framework, so the running binary isn't inside the bundle being replaced. The production case (binary swapped for a differently-signed ShipIt at the same path, user domain, NULL auth) is only covered by the manual probes in the description — fine, just noting it isn't under CI.
| if (job == nil || job[@"PID"] != nil) return nil; | ||
| // ShipIt removes its own launchd job when it finishes successfully, so | ||
| // once the job has been submitted, its disappearance means a clean exit. | ||
| if (job == nil) return @0; |
There was a problem hiding this comment.
nit: this makes "never submitted" and "succeeded and removed" indistinguishable, so a launch failure now sails through SQRLWaitForShipIt and only trips on the later version assertion.
2 participants
Closes #21.
Jobs submitted with
SMJobSubmitare registrations, not one-shots, so after an update the ShipIt job lingers in the launchd domain as "not running" until logout. macOS 27 now surfaces this as a Dock tile on the updated app after the user quits it (see this report).exit(0). Failure exits keep the job soKeepAlivestill retries interrupted installs.AuthorizationCreatefails witherrAuthorizationDeniedeven as root.SMJobRemove(NULL)authorizes on the caller (root → system domain, otherwise the caller's user domain), never prompts, and works with the binary gone. This also means the 2013 concern above about re-prompting for the privileged path doesn't apply.SIGTERMis ignored during removal (launchd TERMs a job it's removing) andwait=falseavoids deadlocking on our own exit.Validated beyond the suite (108/0): a system-domain end-to-end run of the built ShipIt as root — real 1.0→2.1 install with the privileged target guard satisfied — self-removed from the system domain with no authorization prompt, and deleted-binary probes confirm
SMJobRemove(NULL)succeeds in both domains after the running binary is unlinked.