diff --git a/CHANGELOG.md b/CHANGELOG.md index 91617b9..a21ca1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,54 @@ current as you land changes. ## [Unreleased] +### Fixed + +- **"Open minimized" now actually decides whether the window opens.** The app + used to infer a login launch from `NSApplication.launchIsDefaultUserInfoKey`, + which reported wrong in both directions — the window appeared at login with + the setting on, and stayed hidden on a manual launch. The login item is now a + LaunchAgent shipped inside the bundle that passes `--background`, so the app + reads the launch kind instead of guessing it + ([ADR-0014](docs/adr/0014-login-item-launch-marker.md)). The main window also + opted out of AppKit state restoration, which could reopen it at launch without + consulting the setting at all. Existing installs are migrated once, on first + launch; if you had login-at-launch switched off, it stays off. + + Three consequences of the mechanism, handled in the same change. A launchd + agent starts the moment it is registered and — unlike the LaunchServices login + item it replaces — does not check whether the app is already running, so + turning "Open this app at login" on could leave you with two menubar icons; a + duplicate copy now exits at startup, and if you started it yourself it brings + the running copy forward with its window open rather than appearing to do + nothing. A launchd registration also does not disappear with the app bundle + the way a login item did, so `uninstall.sh` now has the app retract it before + deleting anything, instead of leaving an entry that fails to load at every + subsequent login; the Login Items entry reads "Dezhban" now instead of a raw + job label. And the login toggle says what actually happened — including when + macOS is holding the registration for your approval, and when it refuses to + remove the old login item and only you can clear it in System Settings; either + state can still be switched back off, which an earlier build could not do. + + Dezhban also stops relying on macOS's "Reopen windows when logging back in" to + leave it alone: that path relaunches the app at login without the marker, so it + is now opted out of explicitly, leaving the login item as the only thing that + starts the app at login. And clicking the login switch acts on the state you + clicked, not on a re-read a moment later — with the Settings window open, + removing the login item in System Settings and coming back used to make the + next click turn login-at-launch *on*. If you had switched Dezhban off under + System Settings → General → Login Items, upgrading leaves it off; and a copy of + the app run from somewhere other than `/Applications` no longer claims the + login item for a location it is about to be moved out of — and neither does the + Settings switch, which now says so rather than registering a login item that would + break the moment the copy moves. The login switch also + no longer freezes the Settings window while macOS thinks about it, and reads OFF + rather than ON when the item you see is one you had already switched off in + System Settings. Uninstalling clears Dezhban's saved app preferences too — + leaving them behind meant a later install silently skipped the login-item + migration. If macOS refuses to retract the login item — or the app bundle was + already in the Trash, so nothing can — the uninstaller now says so instead of + reporting a clean removal. + ## [0.11.0] - 2026-08-21 ### Added diff --git a/docs/adr/0014-login-item-launch-marker.md b/docs/adr/0014-login-item-launch-marker.md new file mode 100644 index 0000000..94941b4 --- /dev/null +++ b/docs/adr/0014-login-item-launch-marker.md @@ -0,0 +1,501 @@ +# ADR-0014: The login item carries an explicit launch marker + +**Date**: 2026-08-21 +**Status**: accepted +**Deciders**: Behnam RK + +## Context + +The macOS app's "Open minimized" setting (`LaunchVisibility`: never / always / +only at login) needs to know one thing: did macOS start this app at login, or +did the user start it? The `bootOnly` case — the default, and the behaviour the +app had before the setting existed — is defined entirely by that distinction. + +Until now the app asked AppKit, reading +`NSApplication.launchIsDefaultUserInfoKey` from the +`applicationDidFinishLaunching` notification. That key is documented as false +for launches the system performed on the user's behalf, and it was the sole +input to the decision. In practice it read wrong in both directions: the window +appeared at login with the setting on, and failed to appear on a manual Finder +launch. Compounding it, `MainWindow`'s `NSWindow` left `isRestorable` at its +default `true`, so AppKit's state restoration could reopen the window at launch +without consulting the setting at all. + +The app registers itself for login with `SMAppService.mainApp`, which relaunches +the bundle with no arguments and no marker of any kind — so there was nothing +else to consult. Any fix that stayed with `mainApp` would have had to replace +one heuristic with another. + +## Decision + +Register a **LaunchAgent** shipped inside the bundle +(`Contents/Library/LaunchAgents/com.behnam-rk.dezhban.app.login.plist`, via +`SMAppService.agent(plistName:)`) whose `ProgramArguments` end in +`--background`. `LaunchVisibility.isBackgroundLaunch(arguments:)` reads that +marker from `CommandLine.arguments`, and `opensWindow(backgroundLaunch:)` +replaces `opensWindow(deliberateLaunch:)`. `MainWindow` additionally sets +`isRestorable = false` so state restoration cannot reopen the window behind the +setting's back. + +An absent marker reads as a user launch. Existing installs are migrated once, on +launch, by `LoginItem.migrateFromMainAppRegistration()`. + +## Alternatives considered + +### Alternative 1: keep `launchIsDefaultUserInfoKey`, add `NSApp.isActive` as a tiebreak + +A login-item launch does not activate the app; a Finder or Dock launch does. + +- **Pros**: no packaging change at all; ships in one file. +- **Cons**: still a heuristic, and now a compound one. Activation state at + `applicationDidFinishLaunching` races anything else competing for focus at + login, which is precisely the moment the machine is busiest. +- **Why not**: it trades a signal that is wrong sometimes for a signal that is + wrong less often. The setting has a right answer and the app should know it, + not estimate it. + +### Alternative 2: drop the three-way setting for a plain on/off + +"Open the window when Dezhban starts", yes or no. Nothing to detect. + +- **Pros**: the bug becomes unreachable; the least code of any option. +- **Cons**: loses the default behaviour — quiet at boot, visible when you launch + it yourself — which is what almost everyone wants and what the app did before + the setting existed. Every user would have to pick one of two worse options. +- **Why not**: deleting a feature is not a fix for being unable to implement it. + +### Alternative 3: a separate login-item helper application + +A small helper app in `Contents/Library/LoginItems` that launches the main app +with an argument, the pre-`SMAppService` pattern. + +- **Pros**: also deterministic; long-established. +- **Cons**: a second executable to build, sign, version and keep in step. The + agent plist achieves the identical result with a file that has no code in it. +- **Why not**: strictly more machinery for the same guarantee. + +## Consequences + +### Positive + +- The launch kind is a fact, not an inference. `LaunchVisibility` is testable + against a literal argument list rather than an AppKit notification. +- State restoration can no longer reopen the window independently of the + setting. +- Both directions of the original defect are fixed by the same change. + +### Negative + +- The bundle now has a mandatory `Contents/Library/LaunchAgents` payload, and + `build-app.sh` asserts two things about it that no test can reach: that the + plist's `Label` equals the filename it is installed under (launchd rejects a + mismatch, and `SMAppService` reports that only as a status nobody reads), and + that `ProgramArguments` still carry `--background`. Deleting the marker or + renaming the label would otherwise pass `go test`, `swift test` and the build + while silently restoring the original bug. +- **Registering the agent starts a second copy of the app.** This is the real + cost of leaving `SMAppService.mainApp`. `mainApp` is a LaunchServices login + item, and LaunchServices refuses to launch a bundle that is already running; + an agent with `RunAtLoad` is not that — launchd `exec`s + `Contents/MacOS/DezhbanMenu` directly and dedupes nothing. Both callers of + `register()` run while the app is up (the Settings toggle, and the migration + below), so each would leave two menubar items, two Dock tiles, two 1-second + state-file timers and two update checkers — the duplicate carrying + `--background`, so under the default `bootOnly` it opens no window and there + is no way to tell the two icons apart. `RunAtLoad` has to stay true or the + login launch never happens, so the duplicate is caught at startup instead: + `acquireSessionOwnership()` in `main.swift` takes an exclusive `flock` before + `NSApplication` exists, and a process that cannot get it exits. + + A lock rather than "which copy launched first", which is what this was first + written as and which cannot work. Only a *newly started* process ever + evaluates the question — the copy already serving the menubar never + re-evaluates anything — so any rule under which the newcomer might decide it + wins leaves both running, and any rule under which an undatable process yields + can retire both and leave the Mac with no app at all. + `NSRunningApplication.launchDate` is documented as optional, so both failures + were reachable. The kernel has neither problem: exactly one open file + description holds the lock, and it is released when that process dies however + it dies, so a crashed predecessor cannot lock its successor out. The lock is + keyed on the bundle's **path**, not its identifier, because + `dist/Dezhban.app` run against an installed `/Applications/Dezhban.app` is the + documented GUI dev loop and those two are not duplicates of each other. + + The lock file lives under Application Support, not `~/Library/Caches`. `flock` + is per-inode and macOS may purge a caches directory under disk pressure; a + purged lock file while the incumbent holds its descriptor means the next launch + creates a fresh inode, locks *that*, and runs a second copy undetectably. + + What "held by another" is allowed to mean depends on where the lock lives. Locally + `flock` is the kernel's, so held means alive and a launch that finds it taken yields + even before LaunchServices has registered the incumbent — which at login it usually + has not, that being the ordinary case rather than an error. Only on a network home, + where the server emulates the lock and it can outlive its holder, may "nobody is + there" mean the lock is stale and the app start anyway; a timer cannot make that + distinction, and a bounded probe that tried started a second copy on the common + path. + + Only the session owner answers hand-offs. A copy that could not take the lock and + started anyway — the `.unavailable` path, which exists so a broken support + directory cannot stop the app launching — runs beside the real owner, and if both + answered, one double-click would open two windows: each tracks separately which + requests it has answered, so neither can recognise the other's. + + A launch the *user* performed must never become a silent no-op, so the copy + that loses the lock focuses the winner and posts a distributed notification + asking it to open its own, since the incumbent may be a `--background` login + launch with none. + + Not gated on "Open minimized". It was, on the reasoning that "Always" has to mean + always — but the preference governs the *launch*, and a user-initiated launch of + an already-running app is not one: once the incumbent has finished starting, + LaunchServices turns the same double-click into a reopen, which + `applicationShouldHandleReopen` answers unconditionally in every mode, by design. + So the gate bought no consistency — it gave one gesture opposite answers + depending on whether the incumbent had finished starting — and cost the single + launch with no other route to a window. + Scoped by posting the bundle **path** as the notification object, since the + name derives from the bundle id and two installs may legitimately run side by + side. And a notification rather than re-opening the bundle through + `NSWorkspace`: asking LaunchServices to open the app we are in the middle of + quitting could spawn yet another copy, which would find the lock held and ask + again. The observer is registered as the first statement of + `applicationDidFinishLaunching` — distributed notifications are delivered + immediately and never queued, so anything ahead of it is time in which the + hand-off is dropped. + + That is not the whole window, though, and the rest of it needs a file. The lock + is taken before `NSApplication` exists, so between acquiring it and installing + the observer there is a stretch in which a hand-off is posted to nobody — short, + but landing exactly at login, when someone impatient with a slow start + double-clicks the app. So the losing copy writes a `HandoffRequest` beside the + lock as well as posting, and the incumbent consumes it both when it installs the + observer and for a few seconds after — bounded rather than on every tick, + because a permanent per-tick stat on the main thread is the hazard + `pollStateFile` was restructured to remove and this feature is cosmetic; the + window it covers is a launch-time one, and once the observer exists the + notification carries every later hand-off. Both signals describe the same + request, so acting on it is a *claim* — whoever removes the file acts and the + other stands down. Reading the timestamp and removing without checking, which is + what it did first, let the notification handler and the backstop both conclude + they had it and open the window twice: a second `NSApp.activate` half a second + after the first, or a window reopening right after the user closed it. + + The claim settles ownership; it cannot settle everything, and trying to make it + do so was the wrong instinct. The duplicate writes the file and *then* posts, so + the two signals can pass each other in ways where both callers legitimately + conclude they should act — and refusing to act on the ambiguous ones turns a + hand-off into the silent no-op the mechanism exists to prevent, which is the + worse failure of the two. So the notification acts on everything except `.lost`, + and the backstop acts only on `.fresh`. + + Which signal acts is settled by identity, never by timing. Each request carries a + token, written into the file and repeated in the notification, so a signal + describing a request already answered is recognised as such while a genuinely new + launch — new token — always opens. Several answered tokens are remembered, not one: + two duplicates launching inside the backstop window put three signals in flight, + since the second `post()` atomically replaces the first's file, and a single slot + answered one of them twice. + + Three earlier versions inferred this from elapsed time plus a definite/indefinite + flag, and every one of them both let a duplicate window through in one ordering and + swallowed a real second launch in another, because elapsed time does not carry the + information being asked for. There is no time-based debounce anywhere in the + shipped design. + + And accepting an ambiguous notification is bounded to the launch window, because + `DistributedNotificationCenter` is a system-wide bus with no sender + authentication and both the name and the object are derivable. Unbounded, any + process running as this user could call `MainWindow.open()` — which activates the + app — indefinitely, reopening a window the moment it was closed. Requiring a file + outside the launch window costs nothing real (the + file is written before the post, so only the microsecond gap between the two + needs the exemption) and removes the channel — with one addition: a poster whose + file could not be written says so in the notification, because it is then the only + signal there will be and only the poster can know that. Requiring a file + unconditionally turned a full or read-only home into the silent no-op the + mechanism exists to prevent, and the exemption weakens nothing that was not + already reachable, since anything able to forge the notification could equally run + `open -a Dezhban`. A request that cannot be claimed at all is held to the same rule + as no request: since the claim is a rename, that state means the directory is + unwritable, which is also why no poster could have written the file — it is a + leftover, and acting on it turned every later notification into an activation. + + Both consumers do their claim off the main thread, since it is a stat and an + unlink and a network or relocated home would otherwise block the run loop on the + one path meant to feel instant. + + A request the incumbent never got to must not be inherited by the *next* app to + start and turned into a window nobody asked for. That is handled by the session + owner discarding whatever it finds at the moment it takes the lock — exactly, + where the first design guessed with a 30-second age cutoff. The cutoff only added + a way to be wrong, and it was: a cold login where the incumbent takes longer than + that to finish starting is precisely the impatient-double-click case this + mechanism is written around, and the cutoff threw that request away. So a claimed + file is honoured however old it is, because by construction it was written after + this process took the lock. +- **macOS has a second way to start the app at login, and it carries no marker.** + "Reopen windows when logging back in" relaunches whatever was running at + logout, through LaunchServices, with no arguments. `SMAppService.mainApp` was + reconciled with that path because it went through LaunchServices too; a launchd + agent is not, so both would start at login and race for the session lock, and + a resume copy that won made the window open at login under the default + `bootOnly` — this very defect, intermittent instead of absent. + `NSApp.disableRelaunchOnLogin()` is the API for "the login item is the only way + I start at login", and the app calls it at launch. `MainWindow`'s + `isRestorable = false` covers window restoration; this covers app relaunch, + which is a different mechanism. +- **Uninstalling has to retract the registration, and only the app can.** A + LaunchServices login item disappears with its bundle; a per-user launchd agent + does not. `launchctl bootout` is not the answer either — it unloads the job for + the current boot and leaves the record that recreates it at the next login, + pointing at a plist inside a bundle that has been deleted, which is exactly the + orphan being avoided. `SMAppService.unregister()` is the only real retraction + and it can only be called by the app, so `DezhbanMenu` takes a + `--unregister-login-item` errand flag — handled before the session lock, since + it is not a second copy competing for the session — and + `packaging/macos/uninstall.sh` runs it as the console user inside their GUI + session before deleting the bundle. Root cannot reach another account's launchd + session, so other users' entries are named in the closing message instead — as is + the case where there is no logged-in user at all (run at the login window, or + over ssh), where none of the per-user teardown can happen. + + The uninstaller also *looks* for the bundle rather than assuming + `/Applications/Dezhban.app`. The migration is allowed to run from anywhere under + an Applications directory, so a copy filed into `/Applications/Utilities` would + otherwise be told its bundle was already gone — while it went on launching at + every login and the script reported everything removed. + + One thing the app cannot tell the script: whether a registration exists that this + bundle's plist can no longer describe. `SMAppService`'s `.notFound` looks like it + carries that, and on this OS it is simply what an agent that was never registered + reports — verified against the shipped ad-hoc bundle. Treating it as "cannot tell" + made every uninstall of an install where login-at-launch had never been switched on + warn that macOS refused to retract it, and had `disable()` snap the switch back on + over a retraction that had just succeeded. So nothing infers it, and the honest + signal is narrower: an unregister that actually fails. + + The errand's exit status is load-bearing: `unregister()` only logs a refusal and + the script discards the output, so without it a login item macOS would not + retract stayed behind — pointing at a bundle deleted moments later, unreachable + from then on — while the script reported everything removed. Uninstalling also + clears the app's preference domain, which is not the cosmetic cleanup it looks + like: the migration records that it has run, so a surviving flag means a *later* + install is never migrated onto the agent, silently restoring the defect this ADR + is about. + +### Risks + +- **A user who had login-at-launch enabled loses it on upgrade.** + `migrateFromMainAppRegistration()` unregisters `mainApp` and registers the + agent, gated on the old registration having been enabled — so an upgrade never + switches the login item *on* for someone who had it off. + + The attempt is recorded in a persisted flag + (`dezhban.loginItemMigratedToAgent`) and therefore happens at most once per + account, whether or not it worked. Inferring "already migrated" from a live + `mainApp.status` read instead — the first shape of this code — was only + truthful when the unregister had succeeded, and it can fail for real: a login + item added by hand in System Settings was never registered through + `SMAppService`, and unregister is also known to fail after the bundle moves. + The migration then re-ran on every launch and re-registered the agent after + the user had switched login-at-launch off in Settings, leaving it on with no + way to turn it off from the UI. + + The retraction attempt is unconditional, even for a legacy item that is present + but not enabled. Leaving that one alone was the earlier choice, on the reasoning + that the user had switched it off in System Settings and their "off" should not be + undone behind their back — but `.requiresApproval` is a *live* registration, not a + dead one. Re-approving "Dezhban" under Login Items later would have LaunchServices + start the app with no `--background`, breaking "Open minimized" permanently, since + the migration is marked done and never runs again. It was also unreachable: + `isEnabled` asks whether the legacy item is *enabled*, so the switch read OFF and + a click routed to `enable()`, never to the `disable()` that could have cleared it. + Retracting it honours the same "off" and removes the way back to the defect; + whether it *was* enabled still decides whether the agent is then registered. + + When the legacy item survives the attempt — checked by reading its status + after, not by trusting the call not to throw — the agent is deliberately left + unregistered. Registering it anyway would mean two launches at login, the + agent with `--background` and the legacy item without, and whichever won the + race would decide whether the window appeared: worse than the behaviour it + replaces. `LoginItem.isEnabled` reports the legacy registration too, so the + Settings toggle tells the truth about whether anything starts the app at + login, and switching it off retracts *both*. + + The two stuck states are separate outcomes, because they are opposite facts. + From the disable direction the old item is still *enabled*, so something is + starting the app and the switch must stay on. From the enable direction nothing + is registered at all — `enable()` is only entered when the switch read off — so + an on-ish outcome snapped the switch on over a state where nothing was + registered, and the next `seed()` flipped it back: the switch-versus-`isEnabled` + disagreement that must never exist. Sharing one case also had it telling a user + who clicked *off* what switching it *on* would do. + + `enable()` refuses while *any* legacy registration survives, not merely an + enabled one. Guarding on enablement looked like a way to keep the advice below + from being a dead end — a `.requiresApproval` item launches nothing, so it cannot + be half of "two launches at login" — but `AssociatedBundleIdentifiers` makes one + "Dezhban" row in Login Items govern both registrations, so approving that row arms + both. The dead end is real and is stated rather than engineered around: a defect + this branch exists to remove cannot be traded for a better error message. + + If macOS keeps refusing to retract it, the app has no way out on its own, and + it must not pretend otherwise: "toggle it off and on again" was the first + advice here and it was unreachable. The control was a `toggle()` that derived + the direction to move in from `isEnabled`, which the stuck legacy item holds + true — so every attempt took the *off* branch and could never reach + `register()`. The control is `LoginItem.set(enabled:)` now, taking the state the + user asked for, and it returns an `Outcome` rather than a `Bool`; the + `legacyStuck` case tells the user the + one thing that does work: remove "Dezhban" under System Settings → General → + Login Items. Once they do, the toggle registers a clean agent. + + The same `Outcome` carries `awaitingApproval`, because `register()` reports + "the user must approve this in System Settings" as a *status* rather than an + error, and a switch that snaps back with no explanation is indistinguishable + from a bug. `.requiresApproval` is why "is there a registration" and "will this + start the app" have to be separate questions: it is a live registration that + starts the app the moment approval lands, so the unregisters are guarded on the + former. Guarding them on `.enabled` meant an awaiting-approval registration + could not be retracted by the Settings switch *or* by the uninstaller's errand + — the bundle would be deleted with the registration still on file, which is the + orphan the errand exists to remove. `isEnabled` reports that same question, so + what the switch shows agrees with what `set(enabled:)` does; when they + disagreed, an awaiting-approval registration painted the switch ON while + `isEnabled` read "off", and the user's attempt to switch it off re-registered + instead. Taking the requested state rather than re-deriving it also fixed the + stale-switch inversion: the login item is removable in System Settings too, so + a switch left showing ON while that happened turned login-at-launch *on* when + clicked. + + One more thing the persisted flag may not swallow: a migration that retracted + the legacy item and then *failed* to register the agent leaves nothing starting + the app at login. Marking that migrated would cost the user a setting they had + switched on, with no retry ever, so it is deliberately left unmarked and + retried on the next launch. What makes the retry safe rather than a return of + the every-launch re-registration bug is a second flag, set whenever the user + switches login-at-launch off themselves: an explicit "off" outlives every + retry, so a retry can only restore what was already on. + + That third flag records the *attempt*, not the success, and is flushed before the + unregister rather than after it. `SMAppService.mainApp` is itself a launchd job and + the migration's main case is a pre-agent install with login-at-launch on, so the + running app is that job's process and launchd may kill it as the job unloads. + Written afterwards, that kill left the legacy item retracted with nothing recorded + — and the next launch, seeing no legacy item and no flag, concluded there had never + been one, marked the account migrated and returned. Nothing starting the app at + login, permanently, which is the hole the flag exists to close. Recording the + attempt also keeps a failed retraction re-attemptable, since the caller re-reads + the live status either way — and every read taken *after* an unregister is a + retried one. `SMAppService.status` is not documented to keep up with + `unregister()`, and a single read microseconds later is what every "the retraction + failed" report rests on: a lagging status on an upgrading account with + login-at-launch on logged "could not be retracted", marked the account migrated + and returned, so the agent was never registered and the register-retry that exists + for precisely that outcome could never run. The pre-checks — "is there anything to + retract at all" — stay single reads, since nothing has happened yet for a status + to lag behind. + + The same discipline applies to the fact that *decides* what happens next. A + pre-upgrade user who switched Dezhban off under System Settings leaves `mainApp` at + `.requiresApproval` and never touched Dezhban's own switch, so nothing records + their intent — and while "was it enabled" lived only in a local, an interruption + between the retraction and `markMigrated()` left the next launch seeing no legacy + item, seeing the attempt flag, falling through, and registering the agent: + login-at-launch back on for someone who had deliberately turned it off. Their off + is now persisted before anything is retracted. + + That retry needs a third flag to exist at all, which is not obvious and was + got wrong first: by the time `register()` is reached the legacy item is already + confirmed gone, so a retry launch that asks "is there a legacy item to + migrate?" reads *no*, marks itself migrated and returns — never reaching + `register()` again. The promised retry was dead code. A flag recorded at the + moment the legacy item is confirmed retracted, before the register is + attempted, is what distinguishes "this account had a login item and the agent + is not up yet" from "this account never had one". + + The migration decides on `.enabled`, not on "is there a registration". These + are different questions and using one predicate for both was a bug in the + user's favour nowhere: `.requiresApproval` is what `mainApp` reports once the + user has switched Dezhban *off* under System Settings → General → Login Items, + so a migration gated on mere presence treated a deliberate off as something to + carry forward — retracting it and registering the agent, turning + login-at-launch back on during an upgrade, with `userDisabledKey` unable to + help because a pre-upgrade user never set it. The unregister *guards* keep + asking the presence question, because a `.requiresApproval` registration is + still a registration to retract. + + Disabling it is gated the same way, and for the sharper version of the reason: the + agent is registered under a *label* that every copy of the app shares along with + the bundle identifier, so a dev build shows the switch ON because the installed + copy registered the agent, and a click off would retract the installed copy's + registration — silently, since the account-wide "off" is not recorded from an + unstable location either. No legitimate registration originates outside an + Applications directory, so such a copy has nothing there to turn off. + + Enabling the login item from Settings is gated on the install location too, not + only the migration. The waiver it used to carry — "an explicit toggle is the user's + own call" — missed that the consequence is not the user's to undo: `register()` + records the calling bundle, and only that bundle can ever `unregister()`. So + toggling it on from `dist/Dezhban.app`, the dev loop `SessionLock` is path-keyed + specifically to allow, leaves a registration pointing into a bundle the next build + deletes — failing to load at every login, orphaning a row in Login Items, and + retractable by nothing, since the uninstaller only searches the Applications + directories. + + And the migration runs only from `/Applications`, without marking the account + migrated otherwise. `register()` records the plist of the *calling* bundle + (`BundleProgram` is bundle-relative) while the flag is shared by every copy of + the app, so one launch from `~/Downloads` — an upgrader trying the app zip + before moving it — or from `dist/` would point the login agent at a bundle + about to move or be deleted and mark the account done forever. It runs + unattended, so it takes the conservative branch — and the Settings toggle takes + the same one, for the reason given above: the consequence is not the user's to + undo, so an explicit click is no more entitled to claim the login item from a + doomed bundle. + + Two smaller versions of the same "the switch must not lie" rule. + `LoginItem.enable()` refuses to register the agent while a legacy item is live, + which `disable()` and the migration already refused — without it the stuck path + led straight to both being registered, which is the two-launch race. And a + failed *agent* unregister has its own outcome rather than reusing `.failed`, + whose `isOn` is false: `unregister()` swallows its throw, so that combination + painted the switch OFF while the registration was live and the app kept + starting at login. +- **The first logout after upgrading is not covered, once.** + `NSApp.disableRelaunchOnLogin()` is asserted by the *running* app, and the + build being replaced never called it — so at that one logout the system has + already registered the app for a LaunchServices resume relaunch. At the next + login both copies start: the agent's with `--background`, the resume's without. + Either way the window opens under the default `bootOnly` — if the agent wins the + lock, the resume copy loses it, reads as a user launch and hands off, which the + incumbent honours; if the resume copy wins, it opens the window itself. So the + defect this ADR fixes reappears exactly once, on the first login after the + upgrade, and never again. + Not worked around, because every workaround guesses. Suppressing a hand-off for + the first N seconds after login would break the case the hand-off exists for — + an impatient double-click at login is *precisely* that window — and it would + keep doing so on every subsequent login to fix one event that has already + passed. A one-off wrong window beats a permanent rule that throws away real + launches. +- **Switching login-at-launch off may terminate the app.** Unverified, and + listed here rather than worked around because the workarounds are worse than + the symptom. `SMAppService.unregister()` unloads the job from the launchd + domain, and launchd terminates a loaded job's running process — which, in a + session the agent started, is the app itself. So switching the Settings toggle + off from a login-started session may quit the app before it can show the + result. It is a nuisance rather than a risk: the daemon is what enforces, the + GUI is a status and control surface, and relaunching restores it. Routing the + agent through `/usr/bin/open` instead would sidestep it and hand the dedupe + back to LaunchServices, but it resolves the bundle by identifier and could + launch a *different* copy of the app than the one that registered — trading a + known nuisance for a silent wrong-bundle launch. There is a manual check for + this in [docs/contribute/testing.md](../contribute/testing.md); if it + reproduces, that trade is worth reopening with measurements rather than + guesses. +- **The marker could be passed by something other than the agent**, making a + user launch look like a login launch. The only consequence is a window that + does not open, and the Dock icon and "Open Dezhban…" both open it + unconditionally in every mode — the setting can never make the window + unreachable. diff --git a/docs/adr/README.md b/docs/adr/README.md index 61dbc48..2d76a1b 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -25,6 +25,7 @@ New records use [template.md](template.md) and take the next free number. | [0011](0011-biometric-enrollment-requires-a-signed-build.md) | Biometric token enrollment requires a signed build, so unsigned builds must refuse it | accepted, implemented (Alternative 3 superseded by 0012) | | [0012](0012-app-checked-biometrics-on-unsigned-builds.md) | App-checked biometrics on unsigned builds, rather than no biometrics | accepted, implemented | | [0013](0013-geo-provider-pass-opt-out.md) | The geo-provider pass gets an opt-out, not a redesign | accepted, implemented | +| [0014](0014-login-item-launch-marker.md) | The login item carries an explicit launch marker | accepted, implemented | > **0006 is the one to read first if you are touching the geo lookup.** It records why > the obvious implementation silently defeats the exit-country check, and it exists diff --git a/docs/concepts/glossary.md b/docs/concepts/glossary.md index f9a9c80..614a1bf 100644 --- a/docs/concepts/glossary.md +++ b/docs/concepts/glossary.md @@ -261,6 +261,33 @@ available, root-only, and independent of the socket. root, **with no daemon running**. Deliberately not a socket operation, because the escape hatch must never depend on the thing it is escaping from. +**Launch marker** — the `--background` argument the macOS app's login LaunchAgent +passes and nothing else does, so the app knows macOS started it at login rather +than the user starting it. What the "Open minimized" setting decides on; before +it, the app inferred the launch kind from an AppKit key that read wrong in both +directions ([ADR-0014](../adr/0014-login-item-launch-marker.md)). + +**Login agent** — `Contents/Library/LaunchAgents/com.behnam-rk.dezhban.app.login.plist` +inside `Dezhban.app`, registered with `SMAppService.agent(plistName:)`. It is what +starts the app at login, and it exists in place of `SMAppService.mainApp` solely +because a LaunchAgent can pass the **launch marker**. Unlike the login item it +replaced it does not disappear with the bundle, so `uninstall.sh` has the app +retract it. + +**Session lock** — an exclusive `flock` the **macOS app** holds for its lifetime, +one per install, so a second copy of the same bundle exits at startup instead of +running a second menubar item, Dock tile and state-file timer. Needed because +registering the login agent starts the app immediately and launchd, unlike +LaunchServices, does not care that it is already running. Distinct from the +**single-instance lock** below, which is the daemon's and guards `Backend.Apply`; +this one guards nothing but the app's own uniqueness. + +**Hand-off request** — a file beside the **session lock** by which a copy of the +app that is exiting asks the copy that owns the session to show its window, so a +launch the user performed is never a silent no-op. A file rather than only a +notification because the notification is never queued and the owner may not be +observing yet. + **Single-instance lock** — an exclusive lock `run` holds over the state directory for its entire lifetime, so a second `run` — with or without `--no-daemon` — refuses outright instead of racing the first to call `Backend.Apply`. Released diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index ea73ab9..68cc56b 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -757,8 +757,231 @@ task gui:build && open dist/Dezhban.app still works with the daemon stopped and with the main window unable to open. - [ ] **Window opening.** "Open Dezhban…" and a Dock-icon click both open/focus - the main window; a fresh app launch opens **no** window (menubar + Dock - only); closing the window (⌘W) leaves the app and icon running. + the main window; closing the window (⌘W) leaves the app and icon running. + Both work in **every** "Open minimized" mode — the preference governs the + launch only and must never make the window unreachable. +- [ ] **"Open minimized" honours the setting** + ([ADR-0014](../adr/0014-login-item-launch-marker.md)). With "Open this app + at login" on, for each mode: **Only at login** (the default) → log out and + back in, **no** window; then launch from Finder, window opens. + **Always** → no window either way. **Never** → window both ways. The + marker is what makes this work, so also confirm the login launch carries + it: `ps -o args= -p "$(pgrep -x DezhbanMenu)"` ends in `--background` + after a login launch and does not after a Finder launch. +- [ ] **Login-item migration is one-way and never opts you in.** On an install + that predates the agent: with login-at-launch **on**, launch once, then + confirm `SMAppService.mainApp` is no longer registered while the agent is + (`launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` succeeds) and + the Settings toggle still reads on. Repeat with login-at-launch **off**: + it must still be off, and the agent must not be registered. +- [ ] **State restoration cannot reopen the window.** With the window open and + "Close windows when quitting an application" *unchecked* in System + Settings → Desktop & Dock, quit and relaunch in a mode that should open no + window — it must stay closed. +- [ ] **Registering the login item does not leave two apps running.** With the + app up, switch Settings → "Open this app at login" **off then on**. The + agent's `RunAtLoad` execs a second copy the moment it registers, and + launchd does not dedupe the way LaunchServices did, so this is the check + that the session lock works: exactly **one** menubar item and one Dock + tile afterwards, and `pgrep -x DezhbanMenu | wc -l` is 1. Repeat + immediately after an upgrade that runs the migration. +- [ ] **Launching a fully-started app again just reopens its window.** With the + app already running from a `--background` login launch, launch it from + Finder. LaunchServices will not start a second copy of a running bundle, so + this never reaches the session lock — it is + `applicationShouldHandleReopen`, which opens the window in **every** "Open + minimized" mode, on purpose: the preference governs the launch, and must + never make the window unreachable. Do not expect "Always" to suppress it + here; the hand-off path is exercised by the startup-race check below, which + is the only way to reach it. +- [ ] **The first logout after upgrading may open the window once — and only + once.** Expected, not a regression: + [ADR-0014](../adr/0014-login-item-launch-marker.md) records why + `NSApp.disableRelaunchOnLogin()` cannot cover the logout that happened before + the new build ever ran. Upgrade, log out, log back in: a window here is + acceptable. Log out and back in a *second* time — there must be none, and + `pgrep -x DezhbanMenu | wc -l` must be 1. +- [ ] **"Reopen windows when logging back in" does not start a second, unmarked + copy.** Check that box in System Settings → Desktop & Dock, leave the app + running, log out and back in. Exactly one copy must be running and it must + have come from the login agent, not the resume: `ps -o args= -p "$(pgrep -x + DezhbanMenu)"` ends in `--background`, and under the default "Only at + login" there is no window. This is `NSApp.disableRelaunchOnLogin()`; without + it, LaunchServices relaunches the app with no arguments and races the agent + for the lock. +- [ ] **A refusal's explanation survives switching away and back — and expires + when it stops being true.** Run `dist/Dezhban.app`, click "Open this app at + login" — it refuses and says why. Click another app and click back: the + explanation must still be there. Only messages about a moment that has passed + ("macOS is holding this for your approval") may be cleared by that refresh; a + switch that snapped back with the reason erased is indistinguishable from a + bug. Then clear the condition and return — once the switch moves, the stale + refusal must go with it rather than sit there contradicting it. +- [ ] **The login toggle's result has its own line.** Start the service toggle + ("Start the guard at boot"), and while its privileged sequence is running flip + "Open this app at login". Both messages must be readable at once — the install's + progress on the pane's status line, the login result underneath the toggle — + and neither may erase the other. +- [ ] **Two hand-offs in quick succession both open the window.** Only reachable + while the incumbent is still starting — once it is up, LaunchServices reopens + rather than launching a duplicate, so there is no hand-off to debounce. Log + out and back in, then double-click the app twice in quick succession as early + as you can, closing the window (⌘W) in between. Both must open. Best-effort + by nature: the deterministic coverage is + `HandoffRequestTests.anOverlappingClaimerIsToldItLost` and + `theClaimCarriesThePostedToken`, since it is the token — not any timing rule — + that tells the two signals for one request from two requests. +- [ ] **A hand-off that arrives before the app is observing still works.** The + race the `HandoffRequest` file exists for: log out and back in and + double-click the app in `/Applications` as early as you can, while it is + still starting from the login agent. The window must open — within about + half a second if the notification missed it, from the bounded backstop that + runs for the first few seconds. This must hold on a *slow* login too: the + request is honoured however long the incumbent took to finish starting, + because the file can only have been written after it took the lock. It must + open **once**: no second activation a moment later, and a window you close + right after must stay closed. Then + confirm no `.handoff` file is left in `~/Library/Application + Support/com.behnam-rk.dezhban.app/`. +- [ ] **An app filed into a subfolder of /Applications still migrates.** Move + `Dezhban.app` into `/Applications/Utilities/`, launch it on a pre-agent + install with login-at-launch on: it must migrate. Anywhere under an + Applications directory counts as a place the app will stay; comparing only the + immediate parent left that user's legacy item running with no marker + permanently, reported nowhere. +- [ ] **A copy run from outside /Applications cannot claim *or release* the login + item.** Run `dist/Dezhban.app` and switch "Open this app at login" on: it must + refuse, with the line naming where it is running from, and + `launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` must still fail. + Only the registering bundle can ever retract a registration, so a dev build + that claimed it would leave an orphan nothing can remove once `dist` is + rebuilt. + + Then the other direction, which matters more because it is silent: with the + *installed* copy's login item **on**, run `dist/Dezhban.app` — the switch reads + ON, since the agent is registered under a label every copy shares — and click + it **off**. It must refuse, and `launchctl print + gui/$UID/com.behnam-rk.dezhban.app.login` must still succeed afterwards. A dev + build retracting the installed app's registration is bad enough; doing it + without even recording the user's "off" is worse. +- [ ] **A copy run from outside /Applications does not migrate the login item.** + Unzip `Dezhban-macos.app.zip` to `~/Downloads` on a Mac with a pre-agent + install and login-at-launch on, run it once, quit. The legacy login item + must still be there and `defaults read com.behnam-rk.dezhban.app + dezhban.loginItemMigratedToAgent` must be absent — otherwise the account is + marked done with the agent pointing into `~/Downloads`. Then run the copy in + `/Applications`: that one must migrate. +- [ ] **A login item the user turned off in System Settings stays off across an + upgrade — and is cleared, not left dormant.** With a pre-agent build, switch + Dezhban off under System Settings → General → Login Items (this leaves + `mainApp` at `.requiresApproval`, not unregistered), then upgrade and launch. + Login-at-launch must still be off and no agent registered — *and* the entry + must be gone from Login Items. A dormant `.requiresApproval` item is live: + re-approving it there would start the app at login with no marker, and the + migration will not run again to fix it. +- [ ] **Two quick clicks on the login switch settle on the second one.** Click it + off then immediately on (and the reverse). The final switch state must match + the last click *and* the actual registration — + `launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` agreeing with what + the switch shows. Then reopen the pane to confirm it still agrees. +- [ ] **Switching login-at-launch off from a login-started session.** Log out + and back in so the agent starts the app, then switch Settings → "Open this + app at login" **off**. `SMAppService.unregister()` unloads the launchd job + and launchd terminates a loaded job's process — which here is the app — so + watch for the app quitting instead of showing the status line. Recorded as + an open risk in [ADR-0014](../adr/0014-login-item-launch-marker.md); if it + reproduces, note whether the registration was still retracted. +- [ ] **The migration retries a failed agent registration.** Hard to provoke + honestly: with a pre-agent install and login-at-launch on, make + `register()` fail once (an unsigned bundle is the easiest way), launch, and + confirm the log says it will retry. Then fix the bundle and launch again — + the agent must register. `defaults read com.behnam-rk.dezhban.app + dezhban.loginItemMigratedToAgent` must be absent or 0 between the two. +- [ ] **An awaiting-approval registration is explained on a fresh pane open.** With + the agent registered but switched off under System Settings → General → Login + Items, quit Dezhban and reopen Settings *without touching the switch*. It reads + ON (an awaiting-approval registration counts as registered), so the line + explaining that must be there unprompted — nothing else can express the + difference. +- [ ] **The uninstall errand does not cry wolf.** On an install where + login-at-launch was never switched on, run + `Dezhban.app/Contents/MacOS/DezhbanMenu --unregister-login-item; echo $?` — it + must be **0**. `SMAppService` reports `.notFound` for an agent that was never + registered, not only for a plist it cannot resolve, so anything treating that + status as "cannot tell" warns on every ordinary uninstall. +- [ ] **The approval prompt's own guidance survives it.** Turn the login item off + *in System Settings*, then switch Dezhban's "Open this app at login" on: the + line says macOS is holding it for your approval. Click away and back **without + approving** — the line must still be there, because the switch reads ON either + way and cannot show the difference. Then approve it and return: the line must + go. +- [ ] **An awaiting-approval registration can still be switched off.** Turn the + login item off *in System Settings* (not in Dezhban), then switch Dezhban's + "Open this app at login" on: the status line must say macOS is holding it + for approval, and the switch must then turn **off** again on the next click + rather than re-registering. `launchctl print + gui/$UID/com.behnam-rk.dezhban.app.login` must fail afterwards. +- [ ] **The dev build is not deduped against the installed one.** With + `/Applications/Dezhban.app` running, `task gui:build && open + dist/Dezhban.app`. Both must run — the lock is keyed on the bundle path + precisely so every other manual check on this list tests the build you just + made rather than silently testing the installed copy. +- [ ] **The login item is attributable.** System Settings → General → Login + Items shows the entry as **Dezhban**, not as + `com.behnam-rk.dezhban.app.login` (that is `AssociatedBundleIdentifiers` + doing its job — this is the switch a user reaches for to stop the app + starting at login, and it is useless if nobody can tell what it governs). +- [ ] **No `.claiming-*` files accumulate.** After exercising hand-offs, and after a + forced quit during one, `ls -a ~/Library/Application Support/com.behnam-rk.dezhban.app/` + must show no `.claiming-*` leftovers — a process killed between the rename and + the read leaves one, and only the next session owner's sweep removes it. +- [ ] **Uninstall handles two installs.** Put a copy in `/Applications` *and* one in + `~/Applications`, let the second register the login agent, then run the + uninstaller. Both bundles must be gone, the agent registration must be + retracted (`launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` fails, + still after a reboot), and no clean-removal message may print over a survivor. + Only the registering bundle can retract, so each copy's errand has to run from + that copy while it still exists. +- [ ] **Uninstall finds the app where it actually is.** Move `Dezhban.app` into + `/Applications/Utilities/`, let it register the login agent, then run the + uninstaller. It must locate the bundle there, retract the agent, delete it, + and print **no** "app bundle was already gone" warning — the app is allowed to + register from anywhere under an Applications directory, so the uninstaller has + to look in the same places. +- [ ] **Uninstall with nobody logged in says so.** From an ssh session on a Mac + sitting at the login window, run the uninstaller. It must finish *and* warn + that the per-user leftovers could not be removed — every step of that + teardown needs the user's own launchd session, and reporting a clean removal + would hide both a surviving login item and the migration flag that makes a + later reinstall skip the migration. +- [ ] **Uninstall over an already-trashed app says so.** Drag + `/Applications/Dezhban.app` to the Trash, then run the uninstaller. It must + finish *and* print the warning naming System Settings — only the app can + retract its own registration, so with the bundle gone the entry cannot be + removed by anything and reporting a clean uninstall would hide it. +- [ ] **Uninstall retracts the registration, not just the running job.** With + login-at-launch on, run `sudo sh /usr/local/share/dezhban/uninstall.sh`, + then confirm `launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` + fails **and** the System Settings → General → Login Items entry is gone, + **and** that it is still gone after a reboot. A per-user launchd agent does + not go away with its bundle the way a LaunchServices login item did, and + `launchctl bootout` alone only unloads it for the current boot — the + reboot is what distinguishes a real retraction (the + `--unregister-login-item` errand the script runs as the console user) from + an unload that comes back. `defaults read com.behnam-rk.dezhban.app` must + also fail afterwards: a surviving migration flag means a later install is + never moved onto the agent. And if macOS *does* refuse the retraction, the + script must say so — the closing warning naming System Settings, not a clean + "files deleted". +- [ ] **The login agent registers from an ad-hoc-signed build.** Only reachable + on a real install: `build-app.sh` signs with `codesign -s -`, and + `SMAppService.agent` registration goes through launchd's validation of the + bundle, which `SMAppService.mainApp` never needed. If ad-hoc does not + satisfy it, login-at-launch fails as a silent `.notFound`/`.requiresApproval` + status rather than a crash — so check + `launchctl print gui/$UID/com.behnam-rk.dezhban.app.login` on a build + installed the way users get it (`.pkg` or the app zip), not on + `dist/Dezhban.app` run in place. - [ ] **Posture tracking.** Drive the daemon with `--simulate-country IR` / `US` and confirm the menu bar icon *and* the Dock tile flip red/teal and the window's Overview updates within ~1 s. @@ -846,8 +1069,10 @@ end up typing a password. - [ ] **Start at boot** reflects whether the service is registered, flips after install/uninstall (one prompt each, uninstall confirms first), and the uninstall tears rules down before unload. -- [ ] **Launch at login** toggles `SMAppService.mainApp.status` to `.enabled`, and - the app relaunches after a logout/login cycle. +- [ ] **Launch at login** — the login-item checks live with the launch-marker + block earlier in this file, since they are the same mechanism; the switch + registers the *agent* and a correct run leaves `SMAppService.mainApp` + unregistered. - [ ] Guard fields seed from `dezhban config show` values; Apply raises the restart-warning choice; "Save only" writes without restarting. - [ ] **Restart dezhban…** works with nothing else pending: a plain "are you diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 4c77385..cd15350 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -646,7 +646,10 @@ The main window's sidebar sections: (Install service… / Guard up). - **Settings** — startup ("Start the guard at boot" installs the launchd system service so enforcement survives reboots; "Open this app at login" via - `SMAppService`; **"Open minimized"** — Never / Always / Only at login, an + `SMAppService`, registering a LaunchAgent inside the bundle that starts the + app with `--background` so it can tell a login launch from one you asked for + ([ADR-0014](../adr/0014-login-item-launch-marker.md)); **"Open minimized"** — + Never / Always / Only at login, an app-local preference that decides whether the main window opens when Dezhban starts, defaulting to "Only at login", which is what the app always did; the Dock icon and the menubar's "Open Dezhban…" open it regardless; diff --git a/gui/macos/LoginAgent.plist b/gui/macos/LoginAgent.plist new file mode 100644 index 0000000..8bc72ba --- /dev/null +++ b/gui/macos/LoginAgent.plist @@ -0,0 +1,41 @@ + + + + + + Label + com.behnam-rk.dezhban.app.login + + BundleProgram + Contents/MacOS/DezhbanMenu + + ProgramArguments + + Contents/MacOS/DezhbanMenu + --background + + RunAtLoad + + + KeepAlive + + LimitLoadToSessionType + Aqua + + AssociatedBundleIdentifiers + + com.behnam-rk.dezhban.app + + + diff --git a/gui/macos/Sources/DezhbanCore/HandoffRequest.swift b/gui/macos/Sources/DezhbanCore/HandoffRequest.swift new file mode 100644 index 0000000..2116a86 --- /dev/null +++ b/gui/macos/Sources/DezhbanCore/HandoffRequest.swift @@ -0,0 +1,188 @@ +import Foundation + +/// A file beside the session lock saying "a user tried to launch this app; +/// please show yourself". +/// +/// The distributed notification that normally carries this is delivered +/// immediately and never queued, and the incumbent takes the session lock +/// *before* `NSApplication` exists — so between acquiring the lock and installing +/// its observer there is a window in which a hand-off is posted to nobody. It is +/// short, but it lands exactly at login, when a user impatient with a slow start +/// double-clicks the app: the duplicate exits, the notification hits no observer, +/// and their launch does nothing visible at all, which is the one outcome the +/// hand-off exists to prevent. +/// +/// A file closes it because it waits: the incumbent looks for one when it installs +/// the observer, and again for a few seconds after, so a request written during +/// its own startup is still found. The window being covered is a launch-time one, +/// which is why the looking is bounded rather than permanent — once the observer +/// exists the notification carries every later hand-off. +/// +/// Both signals describe the *same* request, so acting on it is a claim rather +/// than a read: whoever removes the file acts, and everyone else stands down. A +/// plain check-then-remove let the notification and the file-watcher both open the +/// window — a second `NSApp.activate` half a second after the first, or a window +/// reopening right after the user closed it. +public struct HandoffRequest { + public let url: URL + + public init(url: URL) { + self.url = url + } + + /// Derived from the lock's own URL, so it is scoped per install for exactly + /// the reasons the lock is (see `SessionLock.forBundle`). + public static func beside(lock: URL) -> HandoffRequest { + HandoffRequest(url: lock.deletingPathExtension().appendingPathExtension("handoff")) + } + + /// Records a request under `token`, reporting whether it landed. + /// + /// Still best effort — a failure must never stop the losing process from + /// exiting — but not silent. If this fails while the incumbent is between + /// taking the lock and installing its observer, which is the exact gap the file + /// exists to cover, then neither signal arrives and the user's launch is the + /// silent no-op the mechanism was written to prevent. The caller cannot repair + /// that, but it can say so. + @discardableResult + public func post(token: String) -> Result { + do { + try Data(token.utf8).write(to: url, options: .atomic) + return .success(()) + } catch { + return .failure(error) + } + } + + /// The result of trying to take a request. + /// + /// There is deliberately no "too old to act on" case. An age cutoff was the + /// first design, to keep a request its intended reader never got from being + /// inherited by the *next* app to start and turned into a window nobody asked + /// for — but `discard()` at lock acquisition already does that, exactly rather + /// than by guessing, so the cutoff only added a way to be wrong. And it was: a + /// cold login where the incumbent takes longer than the cutoff to finish + /// starting is precisely the "user impatient with a slow start" case this whole + /// mechanism is written around, and the cutoff threw that request away. + public enum Claim: Equatable { + /// Taken, carrying the token the poster wrote. Because the session owner + /// discards whatever it finds when it takes the lock, a request seen after + /// that was written by a live duplicate. + /// + /// The token is what makes "two signals for one request" distinguishable + /// from "two requests" *exactly*. Three earlier attempts inferred it from + /// timing and a definite/indefinite flag, and each one both let a duplicate + /// window through and swallowed a genuine second launch, because elapsed + /// time cannot tell those apart. Identity can: the notification carries the + /// same token, so whichever signal arrives second is recognised and dropped + /// while a new launch — new token — always opens. + case fresh(token: String?) + /// There was nothing to take. + case absent + /// There was a request, and somebody else took it first. Whoever did is + /// acting on it, so this caller must not. + case lost + /// There was a request and it could not be removed for a reason other than + /// somebody else having taken it — an unwritable directory, most likely. + /// + /// Its own case because folding it into `.lost` made a *permanent* failure + /// indistinguishable from a benign race: every claimer forever reported + /// "somebody else has this", every one stood down, `discard()` could not + /// clear it either, and the hand-off mechanism was dead for every future + /// launch with nothing said. Callers still stand down — acting on a file + /// that cannot be removed would repeat on every check — but they log it. + case blocked(String) + } + + /// Removes any request without acting on it. + /// + /// Called by a process that has just *taken* the lock: anything on disk at that + /// moment predates its ownership and was meant for a predecessor. This is what + /// makes an age cutoff unnecessary — and it is exact where a cutoff was a guess. + /// The residual window is the microseconds between taking the lock and this + /// call, in which a duplicate could post a request that then gets discarded; + /// orders of magnitude narrower than the launches a 30-second cutoff threw away. + public func discard() { + try? FileManager.default.removeItem(at: url) + } + + /// Removes `.claiming-*` files abandoned by a process that died mid-claim. + /// + /// `claim()` renames the request aside and then reads it, so a kill in between — + /// which this app documents as a real possibility, since retracting a login item + /// can have launchd terminate it — leaves a file `discard()` will never touch, + /// because that only knows the `.handoff` path. Swept by whoever takes the + /// session lock, alongside the discard, since anything found then belongs to a + /// predecessor. + public func sweepAbandonedClaims() { + let dir = url.deletingLastPathComponent() + let prefix = Self.claimPrefix(for: url) + guard let names = try? FileManager.default.contentsOfDirectory(atPath: dir.path) else { + return + } + for name in names where name.hasPrefix(prefix) { + try? FileManager.default.removeItem(at: dir.appendingPathComponent(name)) + } + } + + /// Claim files are named per *request*, not just "a claim in this directory". + /// + /// The directory is shared by every install of the app — `SessionLock` is + /// path-keyed precisely so `dist/Dezhban.app` may run beside the installed copy — + /// so a directory-wide sweep let one install delete the other's in-flight claim. + /// The victim's read then returned nothing, its claim reported a nil token, the + /// token was never recorded, and the paired notification opened the window a + /// second time: the duplicate activation the token design exists to eliminate, + /// caused by the cleanup meant to be harmless. + static func claimPrefix(for url: URL) -> String { + ".claiming-" + url.deletingPathExtension().lastPathComponent + "-" + } + + /// Tries to take the request, and reports whether this caller owns it. + /// + /// The removal is what makes it a claim: exactly one `removeItem` can succeed + /// for a given file, so exactly one caller is told `.fresh` and the other is + /// told `.lost`. Reading the timestamp first and removing afterwards without + /// checking — the first shape of this — let a background check and the + /// notification handler both conclude they had it. + /// + /// `interleaved` exists so the `.lost` branch can be tested at all. It is the + /// one outcome that only occurs when two claimers overlap, and it is also the + /// one that matters — it is what stops both of them acting — so asserting it + /// in a comment rather than a test would be asserting the whole point. + public func claim(interleaved: () -> Void = {}) -> Claim { + guard FileManager.default.fileExists(atPath: url.path) else { return .absent } + interleaved() + // Claimed by *renaming* it aside, then read from the renamed copy. Reading + // first and unlinking after was a TOCTOU on a claim-by-unlink design: a + // second duplicate's `post()` is an atomic replace, so one landing between + // the read and the unlink had this caller delete request T2 while reporting + // token T1 — and T1 is the token already handled, so the caller matched it, + // declined to open, and that second launch became a silent no-op with its + // request gone. The rename is the claim and the read barrier at once. + // + // A request written by an older build carries no token; `nil` then means + // "cannot dedupe this one", which the caller treats as its own identity + // rather than as a match. + let claimed = url.deletingLastPathComponent() + .appendingPathComponent("\(Self.claimPrefix(for: url))\(UUID().uuidString)") + do { + try FileManager.default.moveItem(at: url, to: claimed) + } catch let error as NSError { + // Gone between the check and the rename is the benign case: somebody + // else claimed it and is acting on it. Anything else is a real failure + // and must not wear the same face — see `Claim.blocked`. + if error.domain == NSCocoaErrorDomain, error.code == NSFileNoSuchFileError { + return .lost + } + if let posix = error.underlyingErrors.first as NSError?, + posix.domain == NSPOSIXErrorDomain, posix.code == Int(ENOENT) { + return .lost + } + return .blocked(error.localizedDescription) + } + let token = (try? Data(contentsOf: claimed)).flatMap { String(data: $0, encoding: .utf8) } + try? FileManager.default.removeItem(at: claimed) + return .fresh(token: token?.isEmpty == false ? token : nil) + } +} diff --git a/gui/macos/Sources/DezhbanCore/LaunchVisibility.swift b/gui/macos/Sources/DezhbanCore/LaunchVisibility.swift index d41051d..aaba72e 100644 --- a/gui/macos/Sources/DezhbanCore/LaunchVisibility.swift +++ b/gui/macos/Sources/DezhbanCore/LaunchVisibility.swift @@ -42,21 +42,38 @@ public enum LaunchVisibility: String, CaseIterable, Identifiable, Sendable { /// Whether to open the main window for this launch. /// - /// `deliberateLaunch` is AppKit's `launchIsDefaultUserInfoKey`: false when - /// the launch was performed on the user's behalf (a login item, state - /// restoration, opening a file) rather than at their request. A missing - /// flag reads as an ordinary launch, so the window still opens if AppKit - /// ever stops reporting it — the caller supplies that default. + /// `backgroundLaunch` is true when macOS started the app at login rather + /// than the user starting it. It is read from `--background` in + /// `CommandLine.arguments`, which only the login LaunchAgent passes (see + /// `LoginItem` and docs/adr/0014-login-item-launch-marker.md) — an explicit + /// marker, not a heuristic. The predecessor asked AppKit's + /// `launchIsDefaultUserInfoKey` and got the answer wrong in both + /// directions: the window appeared at login and failed to appear on a + /// manual launch. + /// + /// The absent argument therefore reads as a user launch, which is the safe + /// default: the worst case is a window the user did not ask for, never a + /// window they cannot reach. /// /// Note this governs the LAUNCH only. The Dock icon and the menubar's /// "Open Dezhban…" open the window regardless, in every mode — a /// preference about startup noise must never become a way to lose access /// to the window. - public func opensWindow(deliberateLaunch: Bool) -> Bool { + public func opensWindow(backgroundLaunch: Bool) -> Bool { switch self { case .never: return true case .always: return false - case .bootOnly: return deliberateLaunch + case .bootOnly: return !backgroundLaunch } } + + /// The launch marker the login LaunchAgent passes. Public so the app and + /// its tests name the same string, and so a rename cannot drift away from + /// `LoginAgent.plist`. + public static let backgroundArgument = "--background" + + /// Reads the marker out of a process argument list. + public static func isBackgroundLaunch(arguments: [String]) -> Bool { + arguments.contains(backgroundArgument) + } } diff --git a/gui/macos/Sources/DezhbanCore/SessionLock.swift b/gui/macos/Sources/DezhbanCore/SessionLock.swift new file mode 100644 index 0000000..76fe686 --- /dev/null +++ b/gui/macos/Sources/DezhbanCore/SessionLock.swift @@ -0,0 +1,154 @@ +import Foundation + +/// An advisory `flock(2)` on a per-install lock file: whoever holds it owns this +/// login session, and a second copy of the same app exits. +/// +/// This exists because the login item is a launchd agent (see `LaunchVisibility` +/// and docs/adr/0014-login-item-launch-marker.md). `SMAppService.mainApp` was a +/// LaunchServices entry, and LaunchServices refuses to start a second copy of a +/// bundle that is already running. An agent with `RunAtLoad` is not that: launchd +/// `exec`s `Contents/MacOS/DezhbanMenu` directly, bypassing that dedupe. So +/// `SMAppService.agent(...).register()` — called from the Settings toggle and from +/// the one-shot migration, both while the app is running — starts a *second* app +/// right then: two menubar items, two Dock tiles, two 1-second state-file timers, +/// two update checkers. `RunAtLoad` has to stay true or the login launch never +/// happens, so the duplicate is caught at startup instead. +/// +/// A lock rather than a comparison of the running copies, which is what this was +/// first written as. Comparing "who launched earlier" cannot work here: only a +/// *newly started* process ever evaluates the question, and the copy already +/// serving the menubar never re-evaluates anything. Any rule under which the +/// newcomer might decide it wins leaves both running, and any rule under which an +/// undatable process yields can — with two copies racing — retire both and leave +/// the Mac with no app at all. `NSRunningApplication.launchDate` is documented as +/// optional, so both failures were reachable. The kernel has none of these +/// problems: exactly one open file description holds an exclusive `flock`, and it +/// is released when that process dies, however it dies, so a crashed or +/// force-quit predecessor cannot lock its successor out. +/// +/// Keyed on the bundle's **path**, not its identifier. Two builds of the same app +/// in different places are not duplicates of each other — `dist/Dezhban.app` run +/// against an installed `/Applications/Dezhban.app` is the documented GUI dev loop +/// (docs/contribute/testing.md), and an identifier-scoped lock would have made the +/// freshly built copy exit on launch and silently test the installed one instead. +public final class SessionLock { + public enum Acquisition: Equatable { + /// This process now owns the session and holds the lock until it exits. + case acquired + /// Another live process of the same install holds it. + case heldByAnother + /// The lock file could not be opened at all (unwritable directory, and so + /// on). Treated as `acquired` by callers: refusing to start because a + /// support directory is broken would be a worse bug than a duplicate icon. + case unavailable(String) + } + + /// Where the lock file lives. + public let url: URL + + /// Held for the process's lifetime once acquired. Never closed on purpose — + /// see `release()`. + private var fd: Int32 = -1 + + public init(url: URL) { + self.url = url + } + + /// The conventional location: one file per install, under Application + /// Support. + /// + /// **Not** under `~/Library/Caches`, which is where this first lived. `flock` + /// is per-inode, and macOS is licensed to purge a caches directory under disk + /// pressure — if it removed the file while the incumbent held its descriptor, + /// the next launch's `open(O_CREAT)` would make a *new* inode, take the lock + /// on that, and run a second copy of the app with nothing able to detect it. + /// + /// `bundlePath` is hashed rather than embedded so the name cannot exceed a + /// filename limit, and hashed with FNV-1a rather than `hashValue` because + /// Swift's is seeded per process — two copies of the app must derive the + /// *same* name, which a randomly seeded hash would not give them. + /// Symlinks are resolved before hashing, and must be: two launches of the same + /// install whose `bundleURL` spells differently (`/tmp` against + /// `/private/tmp`, a symlinked install directory) would otherwise derive + /// *different* lock files, both acquire, and both run — the failure this class + /// exists to prevent, arrived at silently. `standardizedFileURL` alone is not + /// enough: it collapses `.` and `..` and expands `~`, and does not touch + /// symlinks. The incumbent match in `acquireSessionOwnership` resolves the + /// same way, for the same reason. + public static func forBundle(path bundlePath: String, + identifier: String, + supportDirectory: URL) -> SessionLock { + let dir = supportDirectory.appendingPathComponent(identifier, isDirectory: true) + try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + let key = URL(fileURLWithPath: bundlePath).resolvingSymlinksInPath().standardizedFileURL.path + let name = "instance-" + String(fnv1a(key), radix: 16) + ".lock" + return SessionLock(url: dir.appendingPathComponent(name)) + } + + /// FNV-1a, 64-bit. Deterministic across processes and OS versions, which is + /// the only property required of it here — this is a name, not a digest. + static func fnv1a(_ s: String) -> UInt64 { + var hash: UInt64 = 0xcbf2_9ce4_8422_2325 + for byte in s.utf8 { + hash ^= UInt64(byte) + hash = hash &* 0x0000_0100_0000_01b3 + } + return hash + } + + public func acquire() -> Acquisition { + guard fd < 0 else { return .acquired } + // O_CLOEXEC because the lock IS this descriptor: a child that inherited it + // would hold the lock past this app's death and lock its own successor out + // — permanently, since nothing would ever release it. Every subprocess the + // app starts goes through Foundation's `Process`, which spawns with + // POSIX_SPAWN_CLOEXEC_DEFAULT on macOS and so does not leak it today. That + // makes this insurance rather than a fix, which is exactly why it belongs + // here: the safety currently rests on an implementation detail of a + // framework, stated nowhere, and the failure it would cause is an app that + // never starts again. + let opened = open(url.path, O_CREAT | O_RDWR | O_CLOEXEC, 0o644) + if opened < 0 { + return .unavailable("open(\(url.path)): \(String(cString: strerror(errno)))") + } + // LOCK_NB: never block. A blocking wait would hang the launch behind a + // process that is not going to exit. + if flock(opened, LOCK_EX | LOCK_NB) != 0 { + let err = errno + close(opened) + if err == EWOULDBLOCK { return .heldByAnother } + return .unavailable("flock(\(url.path)): \(String(cString: strerror(err)))") + } + fd = opened + return .acquired + } + + /// Whether the lock file sits on a local volume, where `flock` is the kernel's + /// and is therefore released when its holder dies. + /// + /// The distinction decides what "held by another" is allowed to mean. Locally it + /// means a live holder, full stop — so a launch that finds the lock taken must + /// yield even when LaunchServices has not yet registered the incumbent, which at + /// login is the ordinary case. On a network home the server emulates the lock and + /// it can outlive the process that took it, and there yielding forever would make + /// the app permanently unstartable. + /// + /// Unknown counts as local: trusting the lock risks a launch that hands off + /// instead of opening, while distrusting it risks two copies of a kill switch's + /// UI, and the first is the smaller failure. + public var isOnLocalVolume: Bool { + let values = try? url.deletingLastPathComponent() + .resourceValues(forKeys: [.volumeIsLocalKey]) + return values?.volumeIsLocal ?? true + } + + /// Drops the lock. Only tests need this: a real process holds the lock until + /// it exits, and the kernel releases it then — including on a crash, which is + /// the whole reason for using a lock rather than a pid file. + public func release() { + guard fd >= 0 else { return } + flock(fd, LOCK_UN) + close(fd) + fd = -1 + } +} diff --git a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift index 2244003..476ea88 100644 --- a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift +++ b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift @@ -20,6 +20,20 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { private let menu = NSMenu() private var timer: Timer? private var updateTimer: Timer? + /// Runs only for a few seconds after launch — see `startHandoffBackstop`. + private var handoffTimer: Timer? + /// Requests already answered, most recent last — see `openForHandoff`. + /// + /// A set rather than a single slot. Two duplicates launching inside the backstop + /// window put *three* signals in flight — D2's `post()` atomically replaces D1's + /// file, so a backstop tick can claim T2 while both notifications still arrive — + /// and with one slot the sequence T2, T1, T2 answered T2 twice: the second + /// activation half a second later that the token design was introduced to + /// prevent, on the exact gesture the checklist asks testers to perform. + private var answeredHandoffTokens: [String] = [] + /// Enough for any plausible burst of interleaved signals, small enough to stay a + /// linear scan of nothing. + private static let answeredHandoffTokenLimit = 16 private var snapshot: Snapshot? private var lastMtime: Date? private var lastIconKey: String? @@ -32,7 +46,48 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { /// a thing to hammer GitHub with. See UpdateChecker's doc comment. private static let updateCheckInterval: TimeInterval = 24 * 60 * 60 - func applicationDidFinishLaunching(_ notification: Notification) { + /// Posted by a duplicate copy of the app as it exits, when the user started + /// it themselves (see `acquireSessionOwnership` in main.swift). Without it a + /// user-initiated launch that loses the session lock would do visibly + /// nothing at all — and the copy that owns the session may be a + /// `--background` login launch with no window to be handed over to. + static let openWindowNotification = "com.behnam-rk.dezhban.app.openWindow" + + /// Set by a poster whose hand-off file could not be written, so it is the only + /// signal there will be. Only the poster knows that, which is why it is carried + /// here rather than inferred by the receiver from a timer. + static let handoffFilelessKey = "dezhban.handoffFileless" + + /// Identifies the request, so the notification and the file-watcher can tell + /// "the other signal for the request I already handled" from "a new launch". + static let handoffTokenKey = "dezhban.handoffToken" + + func applicationDidFinishLaunching(_: Notification) { + // FIRST, and only for the session owner. A duplicate posts its hand-off as + // it exits and then dies; the notification is delivered immediately and + // never queued, so anything ahead of this is time in which a user-initiated + // launch is silently dropped — the one outcome `acquireSessionOwnership` + // exists to prevent. + // + // A copy that could not take the lock and started anyway (the `.unavailable` + // path, for a broken support directory) is running beside the real owner. If + // both answered, one double-click would open two windows, each with its own + // debounce, so neither could suppress the other. + if sessionOwnsLock { + installHandoffHandling() + } + // macOS has a second way to start this app at login, and it does not pass + // the launch marker: "Reopen windows when logging back in" relaunches + // whatever was running at logout, through LaunchServices, with no + // arguments. `SMAppService.mainApp` used to be reconciled with that path + // because it went through LaunchServices too; a launchd agent is not, so + // both would start at login and race for the session lock — and if the + // resume copy won, the window opened at login under the default "Only at + // login", the exact defect this replaced, now intermittent instead of + // absent. This is the API for saying "the login item is the only way I + // start at login". MainWindow's isRestorable = false covers window + // restoration; this covers app relaunch, which is a different thing. + NSApp.disableRelaunchOnLogin() NotificationManager.requestAuthorizationIfNeeded() // Resolve the config path once, off the main thread, before any pane asks for // it — every later read is then a memoized lookup rather than a shell-out on @@ -55,21 +110,31 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { statusItem.menu = menu watchdog.start() refresh() + // Move any pre-agent install onto the login LaunchAgent. Off the main + // thread: it is up to five blocking SMAppService round-trips over XPC plus + // a register() that forks a process, and the moment it runs is a + // slow-to-start login — the exact situation this whole feature is tuned + // around, and the one where a frozen launch is most visible. It only + // affects the NEXT login, so nothing here waits on it. + DispatchQueue.global(qos: .utility).async { + LoginItem.migrateFromMainAppRegistration() + } // Launching the app shows the app: reaching the main window only through // the menubar dropdown made opening it a two-step discovery problem, and // the menubar item stays available either way. // - // Except when the launch wasn't the user's doing. AppKit clears this flag - // for launches it performed on their behalf rather than at their request — - // a login item, a state restoration, opening a file — and a window - // appearing unbidden at every boot is exactly the noise a menubar app - // should not make. A missing flag reads as an ordinary launch, so the - // window still opens if AppKit ever stops reporting this. - let deliberateLaunch = (notification.userInfo?[NSApplication.launchIsDefaultUserInfoKey] as? Bool) ?? true + // Except when the launch wasn't the user's doing. The login LaunchAgent + // passes --background and nothing else does, so this is an explicit + // marker rather than an inference. It replaces + // `NSApplication.launchIsDefaultUserInfoKey`, which was the sole input + // here and read wrong in both directions — the window appeared at login + // and failed to appear on a manual launch. See + // docs/adr/0014-login-item-launch-marker.md. + let backgroundLaunch = LaunchVisibility.isBackgroundLaunch(arguments: CommandLine.arguments) // The Settings "Open minimized" choice decides what to do with that. - // Its default, .bootOnly, is exactly the behaviour described above, so - // anyone who never touches the setting sees no change. - if LaunchPreference.current.opensWindow(deliberateLaunch: deliberateLaunch) { + // Its default, .bootOnly, is the long-standing behaviour — quiet at + // login, visible when you start it yourself. + if LaunchPreference.current.opensWindow(backgroundLaunch: backgroundLaunch) { MainWindow.shared.open() } AppState.shared.refreshServiceState() @@ -83,6 +148,175 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { } } + /// A second copy of the app was started by the user and found this one + /// already owning the session. Opening the window is the whole reason it + /// bothered to tell us — it is standing in for the launch the user performed. + @objc private func openWindowRequested(_ note: Notification) { + // The claim goes off the main thread, like the backstop's: it is a stat and + // an unlink, and on a network or relocated home — the case `uninstall.sh` + // reads NFSHomeDirectory to accommodate — that blocks the run loop on the + // one path that is supposed to feel instant. + // + // Whether the fileless fallback is allowed has to be read here though, + // since it is main-thread state. + // Either the launch window, or the poster telling us its file never landed. + let fileless = (note.userInfo?[Self.handoffFilelessKey] as? String) == "1" + let acceptWithoutFile = handoffTimer != nil || fileless + // The poster's token, so a signal with no file to read still identifies its + // request and cannot be mistaken for a second launch — or for the same one. + let postedToken = note.userInfo?[Self.handoffTokenKey] as? String + DispatchQueue.global(qos: .userInitiated).async { [weak self] in + switch sessionHandoff?.claim() ?? .absent { + case .fresh(let token): + // A file, taken by us. The session owner discards whatever it finds + // when it takes the lock, so a file seen afterwards was written by a + // live duplicate however long ago — which is what makes a slow + // launch work rather than being thrown away for being slow. + DispatchQueue.main.async { self?.openForHandoff(token: token ?? postedToken) } + case .absent: + // No file to point at, so nothing here proves a duplicate of this + // app wrote it. Accepted only while the launch-time backstop is + // armed, which is the only window in which it can legitimately + // happen: the duplicate writes the file and *then* posts, so a + // backstop tick landing between those two calls leaves this with + // nothing to find. + // + // Outside that window a file is required, because + // `DistributedNotificationCenter` is a system-wide bus with no + // sender authentication and both the name and the object are + // derivable. Unbounded, any process running as this user could call + // `MainWindow.open()` — which activates the app — once per debounce + // interval forever, reopening a window the moment it was closed. + // + // The one exemption is a poster that says its file could not be + // written: it is then the only signal there will be, and only the + // poster can know that. It weakens nothing that was not already + // reachable — anything able to forge this could equally run + // `open -a Dezhban` — and without it a full or read-only home turned + // the hand-off into the silent no-op it exists to prevent. + guard acceptWithoutFile else { return } + DispatchQueue.main.async { self?.openForHandoff(token: postedToken) } + case .lost: + // The backstop got there first and is opening the window. + break + case .blocked(let why): + // A request that cannot be claimed proves nothing, so it is held to + // the same rule as no request at all. + // + // Since the claim is a rename, `.blocked` means the directory is + // unwritable — which is also why no *poster* could have written this + // file. It is a leftover, and `discard()` cannot clear it either, so + // acting on it unconditionally turned every later notification into + // an activation: fresh token each time, never matching, window + // reopened indefinitely with no in-product recovery. A real launch in + // that state cannot write its file either, so it arrives with the + // fileless marker and is accepted on that basis instead. + NSLog("DezhbanMenu: hand-off request could not be claimed: \(why)") + guard acceptWithoutFile else { return } + DispatchQueue.main.async { self?.openForHandoff(token: postedToken) } + } + } + } + + /// Opens the window for a hand-off, once per request. + /// + /// `token` identifies the request. The two signals for one request — the + /// notification and the launch-time backstop — carry the same one, so whichever + /// arrives second is recognised and dropped; a genuinely new launch carries a + /// new token and always opens. That distinction has to be exact, because the + /// effect is what the user notices: `MainWindow.open()` activates the app, so a + /// duplicate is a second focus steal or a window reopening just after they + /// closed it, while a dropped one is the silent no-op the whole mechanism exists + /// to prevent. + /// + /// Identity rather than timing, after three attempts at timing. A debounce with + /// a definite/indefinite flag could not tell a paired signal from a new request + /// — elapsed time does not carry that information — so every version of it both + /// let a duplicate window through in one ordering and swallowed a real second + /// launch in another. + /// + /// A `nil` token cannot be matched (a request from an older build, or a poster + /// whose file never landed), so it is always acted on: an extra window is the + /// lesser failure. + private func openForHandoff(token: String?) { + if let token { + if answeredHandoffTokens.contains(token) { return } + answeredHandoffTokens.append(token) + if answeredHandoffTokens.count > Self.answeredHandoffTokenLimit { + answeredHandoffTokens.removeFirst( + answeredHandoffTokens.count - Self.answeredHandoffTokenLimit) + } + } + MainWindow.shared.open() + } + + + /// Installs both hand-off consumers. Called only by the session owner. + /// + /// Scoped to the bundle path, which is what the poster sends: the name comes + /// from the bundle id, and two installs of the app may legitimately run side by + /// side (see `SessionLock`). + private func installHandoffHandling() { + DistributedNotificationCenter.default().addObserver( + self, selector: #selector(openWindowRequested), + name: NSNotification.Name(Self.openWindowNotification), + object: Bundle.main.bundleURL.resolvingSymlinksInPath().standardizedFileURL.path) + // And the file the notification cannot cover: a duplicate that posted while + // this process was still starting up found no observer, so its request is on + // disk. Checked now and for a few seconds more — see startHandoffBackstop for + // why it is bounded rather than on every tick. + startHandoffBackstop() + } + + /// The notification's backstop, for the gap before the observer above exists. + /// + /// Bounded on purpose. The window it covers is a launch-time one — the lock is + /// taken before `NSApplication` — and once the observer is installed the + /// notification carries every later hand-off. Polling the file forever would + /// put a synchronous stat on the main thread on every tick, which is the exact + /// hazard `pollStateFile` below was restructured to remove, for a feature that + /// is cosmetic. So it runs for a few seconds after launch and then stops, and + /// even then it does its filesystem work off the main thread. + private func startHandoffBackstop() { + checkHandoffRequest() + var remaining = 10 + handoffTimer = Timer.scheduledTimer(withTimeInterval: 0.5, repeats: true) { [weak self] timer in + remaining -= 1 + if remaining <= 0 { + timer.invalidate() + self?.handoffTimer = nil + } + self?.checkHandoffRequest() + } + } + + private func checkHandoffRequest() { + guard let handoff = sessionHandoff else { return } + DispatchQueue.global(qos: .utility).async { [weak self] in + // Only `.fresh`. `.lost` means the notification handler claimed it and + // is already opening the window; `.absent` is the ordinary case of + // there being no request at all, which is what almost every tick sees. + var claimed: String? + switch handoff.claim() { + case .fresh(let token): + claimed = token + case .blocked(let why): + // Stand down for real: the comment used to say the backstop stops on + // this while only returning from the one tick, so a 0.5s timer over a + // 5s window logged the same permanent condition eleven times. + NSLog("DezhbanMenu: hand-off request could not be claimed: \(why)") + DispatchQueue.main.async { + self?.handoffTimer?.invalidate() + self?.handoffTimer = nil + } + return + case .absent, .lost: + return + } + DispatchQueue.main.async { self?.openForHandoff(token: claimed) } + } + } + /// Clicking the Dock icon (re)opens the main window — the standard macOS /// contract for a regular app whose windows are all closed. func applicationShouldHandleReopen(_ sender: NSApplication, hasVisibleWindows flag: Bool) -> Bool { diff --git a/gui/macos/Sources/DezhbanMenu/LoginItem.swift b/gui/macos/Sources/DezhbanMenu/LoginItem.swift index c5dffc7..5407d18 100644 --- a/gui/macos/Sources/DezhbanMenu/LoginItem.swift +++ b/gui/macos/Sources/DezhbanMenu/LoginItem.swift @@ -2,28 +2,861 @@ import Foundation import ServiceManagement /// Wraps the menubar app's own login-item registration via SMAppService -/// (macOS 13+). No LaunchAgent plist to ship — the system framework registers -/// the currently-running .app to relaunch at login. Requires a proper bundle -/// with a bundle identifier (assembled by build-app.sh), so it is a no-op / -/// failure when run as a bare SwiftPM binary. +/// (macOS 13+). Requires a proper bundle with a bundle identifier (assembled by +/// build-app.sh), so it is a no-op / failure when run as a bare SwiftPM binary. +/// +/// It registers an **agent** — `Contents/Library/LaunchAgents/…login.plist`, +/// installed by build-app.sh — rather than `SMAppService.mainApp`. The two are +/// equivalent as far as "start me at login" goes; the difference is that the +/// agent's `ProgramArguments` carry `--background`, which is the only reliable +/// way for the app to know that macOS started it rather than the user. See +/// `LaunchVisibility` and docs/adr/0014-login-item-launch-marker.md. +/// +/// Registering an agent `exec`s the app immediately (`RunAtLoad`), so both +/// `set(enabled:)` and the migration below can spawn a second copy of a running +/// app. +/// That is caught at startup by the session lock in main.swift, not here — the +/// duplicate is a *process* problem and this type has no way to see it. enum LoginItem { - static var isEnabled: Bool { - SMAppService.mainApp.status == .enabled + /// Must match `LoginAgent.plist`'s `Label` and the filename build-app.sh + /// installs it under; launchd rejects a mismatch, and SMAppService reports + /// it only as a `.notFound` status. build-app.sh greps this file for the + /// label it installs, so the three cannot drift apart silently. + private static let plistName = "com.behnam-rk.dezhban.app.login.plist" + + /// Whether the one-shot move off `SMAppService.mainApp` has been attempted. + /// + /// Persisted, and deliberately not re-derived from `mainApp.status`: that + /// read is only a truthful "already migrated" signal when the unregister + /// succeeded, and it can fail for real (a login item added by hand in System + /// Settings was never registered through SMAppService; unregister is also + /// known to fail after the bundle moves). Inferring it meant the migration + /// ran again on every launch, which re-registered the agent after the user + /// had switched login-at-launch off in Settings — login-at-launch back on, + /// with no way to turn it off from the UI. Same UserDefaults-flag call as + /// `FirstRun`: a fact about this app on this account, not daemon config. + private static let migratedKey = "dezhban.loginItemMigratedToAgent" + + /// Set when the user switches login-at-launch off themselves. + /// + /// The migration is allowed to retry when it retracted the legacy item but + /// could not register the agent — otherwise that upgrade silently ends with + /// *nothing* starting the app at login and no retry, ever. This flag is what + /// keeps that retry from becoming the bug it replaced: an explicit "off" must + /// outlive it, so a retry never re-registers what the user turned off. + private static let userDisabledKey = "dezhban.loginItemUserDisabled" + + /// Set the moment the legacy item is confirmed gone, before the agent is + /// registered. + /// + /// Without it the retry the comment on `migratedKey` promises is dead code: + /// on the next launch the legacy item is already retracted, so a migration + /// that keys "is there anything to do" off `legacyEnabled` reads false, marks + /// itself migrated and returns — never reaching `register()` again. The user + /// is then left with nothing starting the app at login, permanently, which is + /// the exact outcome the unmarked flag exists to prevent. + private static let legacyRetractionAttemptedKey = "dezhban.loginItemLegacyRetractionAttempted" + + /// What a `set(enabled:)` actually achieved, so the UI can say something + /// true. + /// + /// A plain `Bool` could not: `register()` reports "the user has to approve + /// this in System Settings" as a *status* rather than an error, and the one + /// path where the legacy registration cannot be retracted needs its own + /// message because the app has no way out of it on its own. + enum Outcome: Equatable { + /// Login-at-launch is on, via the agent. + case enabled + /// Nothing will start the app at login. + case disabled + /// Registered, but macOS is holding it for the user's approval — most + /// often because they switched this app off in System Settings before. + case awaitingApproval + /// The legacy LaunchServices login item is still **enabled** and macOS + /// refuses to retract it, so the app still starts at login without the + /// launch marker. Reached only from the disable direction. + case legacyStuck + /// Login-at-launch could not be turned on: an old registration survives + /// that macOS will not remove, and registering the agent beside it would + /// arm two launches at login. Nothing starts the app, so `isOn` is false. + /// + /// Its own case rather than `legacyStuck`, which has `isOn == true`. + /// `enable()` is only entered when the switch read OFF — meaning + /// `isEnabled` was false — so returning an on-ish outcome snapped the + /// switch ON over a state where *nothing* was registered, and the next + /// `seed()` flipped it back. That switch-versus-`isEnabled` disagreement is + /// the one thing `isEnabled`'s docstring says must never exist. + case blockedByLegacy + /// The **agent** registration survived an unregister that failed, so the + /// app still starts at login. + /// + /// Its own case, and `isOn == true`, because the alternative was the one + /// direction of lie that matters: `unregister()` swallows its throw, so + /// this used to come back as `.failed` — `isOn == false` — painting the + /// switch OFF while the registration was live and the app kept starting + /// at login. Exactly what `isEnabled`'s docstring says it exists to stop. + case agentStuck + /// This copy of the app is not somewhere it will stay, so it must not claim + /// the login item. + case unstableLocation(String) + /// Registration failed outright, and nothing is registered. + case failed(String) + + /// Whether this message describes a moment that will pass on its own. + /// + /// `.awaitingApproval` expires the instant the user approves, and the two + /// plain states are legible from the switch itself — those may be cleared by + /// a refresh. The rest explain a *refusal*, and have no other channel: wiping + /// them leaves a switch that snapped back with no reason given, which is + /// indistinguishable from a bug and is the thing this type exists to prevent. + var isTransient: Bool { + switch self { + case .enabled, .disabled: return true + case .awaitingApproval, .legacyStuck, .agentStuck, .blockedByLegacy, + .unstableLocation, .failed: + return false + } + } + + /// Whether this outcome is waiting on the user approving it in System + /// Settings, so its message stops being true the moment they do. + /// + /// `.awaitingApproval` was classed transient on the grounds that a fresh + /// status read lets the switch speak for itself — but the switch cannot + /// express this state: `isEnabled` counts an awaiting-approval registration, + /// so it reads ON while nothing actually starts the app at login. Clearing + /// the message on the next activation therefore left exactly the + /// switch-versus-reality lie this type exists to prevent, and it is the + /// activation the *approval prompt itself* causes. It expires on the status + /// changing, not on a refresh. + var awaitsApproval: Bool { self == .awaitingApproval } + + /// Whether this outcome is a statement about the switch's own state, so that + /// the switch later moving away from it makes the message false. + /// + /// `.unstableLocation` is the only one that does not: it says this copy of the + /// app may not touch the login item, which stays true however the switch + /// reads — and on a dev build beside an installed copy the switch reads ON + /// permanently, so treating it as switch-describing wiped the explanation on + /// the very next activation. + /// + /// `.failed` used to be excluded alongside it, which was wrong in the + /// dangerous direction: the binding moves the switch optimistically to where + /// the user put it and only corrects it from an outcome that describes the + /// switch, so a failed *enable* left it reading ON with nothing registered + /// until the next activation — the switch-versus-reality lie this type exists + /// to prevent. A failure is very much a statement about the switch. + var describesSwitchState: Bool { + switch self { + case .unstableLocation: return false + case .enabled, .disabled, .awaitingApproval, .legacyStuck, .agentStuck, + .blockedByLegacy, .failed: + return true + } + } + + /// Whether anything starts the app at login — what the Settings switch + /// shows. + var isOn: Bool { + switch self { + case .enabled, .awaitingApproval, .legacyStuck, .agentStuck: return true + case .disabled, .failed, .blockedByLegacy, .unstableLocation: return false + } + } + + /// One line for the Settings status area. + var message: String { + switch self { + case .enabled: return "App will open at login." + case .disabled: return "App will not open at login." + case .awaitingApproval: + return "macOS is holding this for your approval — enable Dezhban in " + + "System Settings → General → Login Items." + case .legacyStuck: + // The disable direction: the old item is still enabled, so it is + // still starting the app. Worded for the click the user made — the + // shared wording described switching it *on*, which is not what + // they did. + return "macOS would not remove the old login item, so Dezhban will still open " + + "at login. Logging out and back in usually clears it." + case .blockedByLegacy: + // Worded for neither direction, because both reach it: `enable()` + // refuses here, and `disable()` lands here too when the agent goes + // away but a dormant legacy registration will not. Describing a + // click the user may not have made is the mistake splitting + // `.legacyStuck` out was supposed to end. + return "An old login-item registration is still on file and macOS will not " + + "remove it. Nothing starts Dezhban at login, and switching that on is " + + "blocked while it exists. Logging out and back in usually clears it." + case .agentStuck: + return "macOS would not remove the login item, so Dezhban will still open at " + + "login. Remove \"Dezhban\" under System Settings → General → Login Items." + case .unstableLocation(let where_): + return "Dezhban has to live in Applications to open at login. This copy is " + + "running from \(where_), and a login item pointing there would break the " + + "moment it moves." + case .failed(let why): + return "Could not change the login item: \(why)" + } + } } - /// Toggles login-at-launch. Returns the resulting enabled state; on error it - /// logs and returns the unchanged prior state. - @discardableResult - static func toggle() -> Bool { + /// Every mutation runs here, one at a time. + /// + /// Both callers are off the main thread now — the Settings switch, so a click + /// cannot beachball the window on six blocking XPC round-trips, and the + /// migration, so a slow login is not held up by it — which made them able to + /// interleave. The bad one: the migration passes its `userDisabledKey` check, + /// the user switches login-at-launch off, `disable()` retracts everything, and + /// the migration then registers the agent. Login-at-launch back on immediately + /// after being turned off is the single thing that key exists to prevent, so + /// the two paths cannot be allowed to overlap. + private static let queue = DispatchQueue(label: "com.behnam-rk.dezhban.app.loginitem") + + private static var service: SMAppService { .agent(plistName: plistName) } + + private static var agentEnabled: Bool { service.status == .enabled } + + /// The legacy LaunchServices registration, *enabled* — as opposed to merely + /// present. + /// + /// The distinction decides whether the migration runs at all, and it is not + /// the same question the unregister guards ask. `.requiresApproval` is what + /// `mainApp` reports once the user has switched Dezhban off under System + /// Settings → General → Login Items — so a migration gated on "is there a + /// registration" would treat a deliberate *off* as something to carry + /// forward, retract it, and register the agent: login-at-launch back on + /// behind the user's back, on upgrade, with `userDisabledKey` unable to help + /// because a pre-upgrade user never set it. + private static var legacyEnabled: Bool { SMAppService.mainApp.status == .enabled } + + // There is deliberately no "the plist could not be resolved" predicate, and one + // was tried and removed. `.notFound` looks like it carries that meaning, and on + // this OS it is simply what an agent that was never registered reports — + // verified against the shipped ad-hoc bundle, which exits the retraction errand + // as a failure with nothing registered at all. Treating it as "cannot tell" + // therefore fired on the common path: `disable()` snapped the switch back ON with + // "macOS would not remove the login item" over a retraction that had just + // succeeded, and every uninstall of an install where login-at-launch was never + // switched on warned that macOS had refused to retract it — crying wolf on the + // ordinary path and eroding the signal on the one the warning was written for. + // + // So the distinction is not available from a status read, and nothing pretends + // otherwise. What remains truthful is an unregister that actually fails, which is + // what `registered()` reports afterwards. + + /// Whether a registration exists at all, as opposed to one that will start + /// the app *right now*. + /// + /// The difference is `.requiresApproval`: `register()` leaves the service + /// there when the user has previously switched this app off in System + /// Settings, and it is a live registration that starts the app the moment + /// they approve it. Guarding the unregisters on `.enabled` alone meant an + /// awaiting-approval registration could not be retracted by the Settings + /// switch *or* by the uninstaller's errand — the bundle would be deleted with + /// the registration still on file, which is the orphan the errand exists to + /// remove. + private static func registered(_ target: SMAppService) -> Bool { + isRegistered(target.status) + } + + /// The same question asked of a status already in hand. + /// + /// Every decision that needs more than one fact about the agent takes *one* + /// status read and derives them all from it. Chaining `registered(service)`, + /// `service.status == .requiresApproval` and `agentEnabled` issued three separate + /// XPC queries of a mutable value, so a change between the first and the last + /// selected a branch matching neither instant — the reachable case being a user + /// approving Dezhban in System Settings while the pane read it, getting "macOS is + /// holding this for your approval" written under a switch painted OFF. + private static func isRegistered(_ status: SMAppService.Status) -> Bool { + switch status { + case .notRegistered, .notFound: return false + default: return true + } + } + + /// Whether anything at all is set up to start this app at login — the agent + /// or a legacy registration the migration could not retract. + /// + /// Both, not just the agent: on the failed-migration path the legacy item is + /// still live, so the app still starts at login, and it starts *without* + /// `--background`, which is the very bug this PR fixes. Reporting only the + /// agent would show "off" while startup kept happening, and leave the user + /// no control that reaches the thing launching them. + /// + /// This is the value the switch displays. It must agree with what + /// `set(enabled:)` does, or the two answer different questions about the same + /// switch: an awaiting-approval registration once painted the switch ON while + /// this read "off", so a click meant to disable re-registered instead and + /// there was no way to switch login-at-launch off at all. + /// + /// Asymmetric between the two, and deliberately. For the **agent**, + /// `.requiresApproval` means we registered it and macOS wants the user's + /// consent — on, pending. For the **legacy** item, which this app never + /// registers any more, `.requiresApproval` is the signature of the user having + /// switched Dezhban off under System Settings → General → Login Items — so it + /// launches nothing, and reporting it as ON contradicted the migration's own + /// reading of that exact state ("their 'off' is the answer") and showed a + /// switch that was on while nothing started the app. + /// + /// Read together with `awaitingApproval`, in one call, because they must describe + /// the same instant. The pane keeps an `.awaitingApproval` message on screen + /// until this says the wait is over: `enabled` cannot express that difference, + /// since an awaiting-approval registration counts as registered. + /// + /// On the mutation queue, so a read can never observe a change half-applied. + /// `disable()` retracts the legacy item and then the agent; a read landing + /// between those two saw the agent still registered and reported ON, and if its + /// main-queue hop was enqueued after the mutation's own completion it overwrote + /// the correct answer with that one — the switch reading ON with nothing starting + /// the app at login, which is the failure the revision stamp in `SettingsView` + /// was added for and could not close on its own. + /// + /// This replaced a lone `isEnabled`, which had no callers left once the pane + /// needed the approval fact beside it — and a spare `queue.sync` accessor on this + /// type is an invitation to reintroduce the main-thread beachball it was moved + /// off for. + static var state: (enabled: Bool, awaitingApproval: Bool) { + queue.sync { + let agent = service.status + return (isRegistered(agent) || legacyEnabled, agent == .requiresApproval) + } + } + + /// Sets login-at-launch to `enabled` and reports what actually happened. + /// + /// Takes the state the user asked for rather than deriving it from a live + /// read, which inverted the click whenever the switch was stale: with the + /// Settings pane open, removing the login item in System Settings and then + /// clicking Dezhban's switch — visibly ON, so plainly an attempt to turn it + /// off — read "currently off" and turned login-at-launch on. + /// + /// Turning it OFF retracts both registrations, for the reason `isEnabled` + /// reports both. Turning it ON registers only the agent — the legacy one is + /// never created again. + /// Enqueued, with the result delivered on main. + /// + /// There is no synchronous form. There was, and it had no callers left — a + /// `queue.sync` on a type whose whole job is serializing two mutation paths is + /// an invitation to block the main thread on six XPC round-trips, which is the + /// beachball this was moved off-main to avoid. + /// + /// `queue` is serial, so this preserves click order — which the synchronous + /// form did not when each click was dispatched to a *concurrent* queue and the + /// blocks then raced into `sync`. Two quick clicks could reach the serial queue + /// in the wrong order, leaving the registration in click 1's state while the UI + /// applied click 2's outcome: the switch showing OFF while the app still starts + /// at login, which is the lie the whole `Outcome` type exists to prevent. + static func set(enabled: Bool, completion: @escaping (Outcome) -> Void) { + queue.async { + let outcome = enabled ? enable() : disable() + DispatchQueue.main.async { completion(outcome) } + } + } + + private static func enable() -> Outcome { + // Gated on the install location, exactly as the migration is. The waiver + // this used to carry — "an explicit toggle from Settings is the user's own + // call" — ignored that the consequence is not the user's to undo: + // `register()` records the *calling* bundle (`BundleProgram` is + // bundle-relative), and only the registering bundle can ever call + // `unregister()`. So toggling this on from `dist/Dezhban.app` — the + // documented dev loop, which `SessionLock` is path-keyed specifically to + // allow — leaves a launchd registration pointing into a bundle that the next + // build deletes: it fails to load at every login, leaves an orphan row in + // Login Items, and *nothing in the product can retract it*, since + // uninstall.sh only searches the Applications directories. Same for a zip + // copy run once from ~/Downloads, which is the case the migration's own gate + // exists to avoid. + guard isInStableInstallLocation else { + return .unstableLocation(Bundle.main.bundleURL.deletingLastPathComponent().path) + } + // The agent must never be registered beside a live legacy item — that is + // two launches at login, one with the marker and one without, and + // whichever won the session lock would decide whether the window opened. + // `disable()` and the migration both refuse it; this refused nothing, and + // the stuck-migration path led straight here: switch reads ON, user clicks + // it off, clicks it on again, and both are registered. + // Retract any legacy registration, and refuse while one survives at all — + // not merely while one is *enabled*. + // + // Guarding on enablement was an attempt to keep `.legacyStuck`'s advice + // from being a dead end, on the reasoning that a `.requiresApproval` legacy + // item launches nothing so cannot be half of "two launches at login". But + // `AssociatedBundleIdentifiers` makes ONE "Dezhban" row in Login Items + // govern both registrations, so approving that row arms both — and then the + // agent and the legacy item both start the app, one with the marker and one + // without, racing the session lock to decide whether the window opens. + // That is the defect this whole branch exists to remove, so it cannot be + // traded for a better error message. + // + // The dead end is real and is now stated honestly instead of being + // engineered around: if macOS will not retract the old registration, + // nothing the app can do will make enabling this safe. See + // `Outcome.legacyStuck`. + // The user's intent is recorded and flushed BEFORE the retraction, which is + // the rule the rest of this file follows and this path inverted. + // `retractLegacy()` unloads a launchd job, and in a login-started session + // that job's process is this app — so a kill between the retraction and the + // write left `userDisabledKey` still reading true with the attempt flag now + // set, and the next launch's migration fell straight through to + // `markMigrated()`: the account retired with nothing registered, moments + // after the user asked for login-at-launch on. + UserDefaults.standard.set(false, forKey: userDisabledKey) + UserDefaults.standard.synchronize() + retractLegacy() + if stillRegistered(.mainApp) { + // Which outcome depends on what actually survived, not on the direction + // clicked. A legacy item still *enabled* is starting the app at login, + // so `.legacyStuck` (isOn true) is the true report; only a dormant + // `.requiresApproval` leftover means nothing is starting it. + // + // `.blockedByLegacy` used to be returned for both, on the argument that + // `enable()` is only entered when the switch read OFF so `isEnabled` must + // have been false. That assumes a fresh switch — and the rest of this + // code deliberately treats it as stale, which is exactly why + // `set(enabled:)` takes the state the user asked for instead of + // re-reading. Re-approving the "Dezhban" row in System Settings arms the + // legacy registration behind a switch showing OFF, and clicking it then + // reported "left off" over a live login launch. + return liveOutcome(.enabling, fallback: .blockedByLegacy) + } do { - if isEnabled { - try SMAppService.mainApp.unregister() - } else { - try SMAppService.mainApp.register() + try service.register() + } catch { + NSLog("DezhbanMenu: could not register the login agent: \(error)") + // Re-read rather than assume the throw means nothing is registered. + // This is the same asymmetry `.agentStuck` was added to close on the + // other side: a throw reported as `.failed` has `isOn == false`, so a + // registration that *is* live paints the switch OFF while the app keeps + // starting at login. `kSMErrorAlreadyRegistered` is the obvious way in — + // the switch stale-OFF, the user clicks it on, and the register throws + // precisely because it is already registered. + // + // And the outcome comes from the status, not from the fact that a throw + // happened. Mapping every throw-with-a-registration to `.agentStuck` + // told a user who had just asked to turn login-at-launch ON to go and + // remove the login item — the opposite of what they wanted, in the case + // the comment above names as the main way here. + switch settledStatus() { + case .enabled: return .enabled + case .requiresApproval: return .awaitingApproval + default: return .failed(error.localizedDescription) } + } + // One settled read, then every branch from it. `register()` returns without + // throwing when macOS is going to make the user approve it, so the status is + // the only place that shows up — and the switch snapping back with no + // explanation is indistinguishable from a bug. + let status = settledStatus() + if status == .requiresApproval { return .awaitingApproval } + return status == .enabled ? .enabled : .failed(describe(status)) + } + + private static func disable() -> Outcome { + // Gated exactly as `enable()` is, and for the sharper version of the same + // reason. The agent is registered under a *label*, and every copy of the app + // shares it along with the bundle identifier — so a dev build shows the + // switch ON because the installed copy registered the agent, and one click + // off would retract the *installed* copy's registration. Worse than the + // enable case: it was silent, since the account-wide "off" is not recorded + // from an unstable location either, so the installed app's migration would + // not even see that the user had asked for this. + // + // No legitimate registration originates outside an Applications directory — + // `enable()` and the migration both refuse — so there is nothing here for + // such a copy to legitimately turn off. + guard isInStableInstallLocation else { + return .unstableLocation(Bundle.main.bundleURL.deletingLastPathComponent().path) + } + UserDefaults.standard.set(true, forKey: userDisabledKey) + // Flushed before the unregister below, because that unregister may get + // this process killed: launchd terminates a loaded job's running process, + // and in a login-started session that process is the app (recorded as an + // open risk in docs/adr/0014-login-item-launch-marker.md). UserDefaults + // does not write through synchronously, so losing this one would leave a + // pending migration retry free to register the agent again on the next + // launch — turning login-at-launch back on behind the user, which is the + // single thing this key exists to prevent. + UserDefaults.standard.synchronize() + // Legacy FIRST, agent last, for the same reason the flush above exists: + // the agent unregister is the call that may get this process killed by + // launchd. Done the other way round, a stuck-legacy install in a + // login-started session lost the app between the two lines and left the + // legacy item registered — still starting the app at login, without the + // marker, which is the state this function exists to clear. + retractLegacy() + if registered(service) { unregister(service, what: "login agent") } + if stillRegistered(.mainApp) { + // The stuck path. Reported rather than worked around: registering the + // agent alongside it would mean two launches at login, one with the + // marker and one without, and whichever won the race would decide + // whether the window appeared. `Outcome.legacyStuck` tells the user + // the one thing that does clear it. + NSLog("DezhbanMenu: the legacy login item could not be retracted") + // Same derivation as `enable()`, and via the same helper so the two + // directions cannot drift apart again: they once disagreed about a + // `.requiresApproval` leftover that would not retract — disable reported + // a clean "App will not open at login" and every later click to turn it + // on was refused, permanently, with nothing having warned them. + return liveOutcome(.disabling, fallback: .blockedByLegacy) + } + return stillRegistered(service) ? .agentStuck : .disabled + } + + /// Retracts everything that could start this app at login, best effort. + /// + /// Used by the `--unregister-login-item` errand the uninstaller runs (see + /// main.swift). `SMAppService.unregister()` is the only thing that actually + /// retracts an agent registration — `launchctl bootout` unloads the job for + /// this boot and leaves the record that recreates it at the next login — and + /// only the app can call it. + @discardableResult + static func retractAll() -> Bool { + queue.sync { + if registered(service) { unregister(service, what: "login agent") } + if registered(.mainApp) { unregister(.mainApp, what: "legacy login item") } + // Deliberately NOT through `retractLegacy`: this runs from the uninstall + // errand, where recording "the agent still needs registering" would be a + // lie about an app that is about to be deleted. + // + // Reported, not swallowed. `unregister` only logs its throw, and the + // uninstaller discards this process's output — so a refusal left the + // Login Items entry behind, pointing at a bundle about to be deleted and + // unreachable afterwards, while the script printed "service + // unregistered, files deleted". The orphan the errand exists to remove, + // now silent. The exit status is what makes it visible. + // + return !stillRegistered(service) && !stillRegistered(.mainApp) + } + } + + /// Moves an install that registered `SMAppService.mainApp` (every build + /// before the agent existed) onto the agent, exactly once per account. + /// + /// Gated on the OLD registration being enabled: a user who had + /// login-at-launch switched off must not have it switched on by an upgrade. + /// The attempt is recorded either way, so this runs at most once whether it + /// succeeded or not — see `migratedKey`. + static func migrateFromMainAppRegistration() { + queue.sync { migrateLocked() } + } + + private static func migrateLocked() { + guard !UserDefaults.standard.bool(forKey: migratedKey) else { return } + // Only from a bundle that is going to stay put, and deliberately WITHOUT + // marking migrated — so the installed copy still does this later. + // + // `register()` records the plist of the *calling* bundle (`BundleProgram` + // is bundle-relative) while `migratedKey` is shared by every copy of the + // app, so one launch from ~/Downloads or from dist/ — an upgrader trying + // the app zip before moving it, or a dev build — would point the login + // agent at a bundle that is about to move or be deleted, and mark the + // account done forever. The symptom is an SMAppService status nobody + // reads. `enable()` gates on the same thing, for the reason given there — + // the consequence is not the user's to undo, so an explicit toggle is no + // more entitled to claim the login item from a doomed bundle than this is. + guard isInStableInstallLocation else { + NSLog("DezhbanMenu: not migrating the login item from a non-standard location " + + "(\(Bundle.main.bundleURL.path)); the copy in /Applications will do it") + return + } + let userDisabled = UserDefaults.standard.bool(forKey: userDisabledKey) + + if registered(.mainApp) { + // Whether it was *enabled* decides what happens afterwards — that is + // the user's own on/off — but the retraction attempt itself is + // unconditional, and that is the correction here. + // + // The earlier shape left a present-but-not-enabled item alone, on the + // reasoning that the user had switched it off in System Settings and + // their "off" should not be undone behind their back. But + // `.requiresApproval` is a *live* registration, not a dead one: if they + // later re-approve "Dezhban" under Login Items, LaunchServices starts + // the app with no `--background` and "Open minimized" is broken again — + // permanently, because `migratedKey` is set and this never runs a second + // time. And it was unreachable: `isEnabled` asks `legacyEnabled`, so the + // switch read OFF and a click routed to `enable()`, never to the + // `disable()` the old comment pointed at. Retracting it honours the same + // "off" while removing the way back to the defect. + let wasEnabled = legacyEnabled + if !wasEnabled { + // Their "off" is recorded durably BEFORE anything is retracted, for + // the same reason `retractLegacy` flushes before its unregister — + // except that this is the fact which decides what happens next, and + // it lived only in the local above. + // + // A pre-upgrade user who switched Dezhban off under System Settings + // leaves `mainApp` at `.requiresApproval` and never touched Dezhban's + // own switch, so `userDisabledKey` is unset. If the process ended + // after the retraction was recorded but before `markMigrated()` — a + // quit or a logout during the status read that follows — the next + // launch saw no legacy item, saw the attempt flag set, fell through, + // and registered the agent: login-at-launch switched back ON for + // someone who had deliberately turned it off, which ADR-0014 says + // must never happen. + UserDefaults.standard.set(true, forKey: userDisabledKey) + UserDefaults.standard.synchronize() + } + retractLegacy() + // The retried read, like every other post-unregister check. A single one + // taken microseconds after the unload — which `SMAppService.status` is + // not documented to keep up with — was the worst placement of this bug + // in the file: a lagging status on an upgrading account with + // login-at-launch ON logged "could not be retracted", marked the account + // migrated and returned, so the agent was never registered AND the + // register-retry that exists for exactly that outcome could never run. + // Nothing starting the app at login, permanently, in an NSLog. + if stillRegistered(.mainApp) { + // Stuck. The agent is left unregistered rather than stacked on top + // of a live legacy item — two launches at login, one with the marker + // and one without. Only the user can clear it now. + NSLog("DezhbanMenu: the legacy login item could not be retracted. " + + "Remove \"Dezhban\" under System Settings → General → Login Items; " + + "until then it may start Dezhban at login without the launch marker.") + markMigrated() + return + } + guard wasEnabled else { + // It was off. Nothing is carried forward and the agent is not + // registered; turning login-at-launch on is the user's call. + markMigrated() + return + } + } else if !UserDefaults.standard.bool(forKey: legacyRetractionAttemptedKey) { + // Nothing was ever registered the old way on this account, and no + // retraction was ever attempted, so there is nothing to move onto the + // agent. Turning login-at-launch on is the user's call, via Settings. + markMigrated() + return + } + // Falling through means a retraction was attempted and the legacy item is + // gone — this launch, or an earlier one that was killed by the unload before + // it could finish. + + // An explicit "off" outlives every retry: without this, a migration allowed + // to retry would re-register what the user had switched off. It gates the + // *register* only. Sitting above the legacy block, as it first did, it also + // short-circuited the retraction — which the code twenty lines up insists is + // unconditional, and for a reason: a live `mainApp` registration left on file + // is re-armed by the user approving the single "Dezhban" row under Login + // Items, and then starts the app with no marker, permanently, because the + // migration is marked done and never runs again. + guard !userDisabled else { + markMigrated() + return + } + + // Reached with the legacy item confirmed gone — now, or on an earlier + // launch whose register() failed. + if registered(service) { + markMigrated() + return + } + do { + try service.register() + markMigrated() + } catch { + // Deliberately NOT marked migrated. The legacy item is gone by now, + // so leaving it here means *nothing* starts the app at login — and + // with the flag set that would never be retried, silently costing the + // user a setting they had switched on. The retry is safe because it is + // gated on `userDisabledKey` above: it can only ever restore what was + // already on, never override an explicit "off". + NSLog("DezhbanMenu: could not register the login agent, will retry on next launch: \(error)") + } + } + + /// Retracts the legacy item and records the fact if it worked. + /// + /// The recording is the point. `legacyRetractionAttemptedKey` is what tells "this + /// account had a login item and the agent is not up yet" from "this account + /// never had one", and while only the migration wrote it, retracting through + /// the Settings switch destroyed the fact without recording it — reopening the + /// dead-retry hole from the other side. Switch off (legacy gone, nothing + /// recorded), switch on, `register()` fails, and the next launch sees no + /// legacy item and no flag, concludes there was never anything to migrate, + /// and marks the account done with nothing starting the app at login. + private static func retractLegacy() { + guard registered(.mainApp) else { return } + // Recorded and flushed BEFORE the unregister, not after. `SMAppService.mainApp` + // is itself a launchd job, and the migration's main case is a pre-agent + // install with login-at-launch ON — so the running app *is* that job's + // process, and launchd may terminate it as the job is unloaded. Written + // afterwards, that kill left the legacy item retracted with nothing recorded: + // the next launch saw no legacy item and no flag, concluded there had never + // been one, marked the account migrated and returned. Nothing starting the + // app at login, permanently — the exact hole this flag was added to close. + // + // So it records the *attempt*, not the success. A retraction that fails is + // then re-attempted on the next launch (the caller re-reads `registered` + // either way), while one that succeeded without being recorded is no longer + // mistaken for "there was never anything here". `disable()` already flushes + // before this same call for the same reason. + UserDefaults.standard.set(true, forKey: legacyRetractionAttemptedKey) + UserDefaults.standard.synchronize() + unregister(.mainApp, what: "legacy login item") + } + + private static func markMigrated() { + UserDefaults.standard.set(true, forKey: migratedKey) + // Flushed, because `legacyRetractionAttemptedKey` is. Those two flags are read + // together and one outliving the other inverts the decision they encode: + // this runs seconds into a login, and if the session ended before cfprefsd + // wrote it, a legacy item retracted for a user who had login-at-launch OFF + // left `legacyRetractionAttemptedKey` durable and `migratedKey` gone — so the next + // launch fell through to `register()` and turned it back on, which ADR-0014 + // says must never happen. The mirror loss strands the account with nothing + // starting the app at login. `disable()` already flushes before the call + // that can end the process; this path has the same obligation. + UserDefaults.standard.synchronize() + } + + /// Whether this bundle lives somewhere it is going to stay. + /// + /// `/Applications` is where every shipping path puts it (the `.pkg`, and the + /// app zip, which unpacks straight into it); `~/Applications` is the + /// system-sanctioned per-user equivalent. Anywhere else — `~/Downloads`, + /// `dist/` — is a copy that is about to move or be deleted, and registering + /// the login agent from it would point launchd at a bundle that stops + /// existing. + /// + /// Symlinks are resolved for the same reason `SessionLock` resolves them: an + /// install reached through a symlinked directory is still that install, and a + /// literal string comparison silently answered "no" and left the migration + /// undone forever, reported only in a log line. + private static var isInStableInstallLocation: Bool { + let bundle = Bundle.main.bundleURL + .resolvingSymlinksInPath() + .standardizedFileURL + .deletingLastPathComponent() + .path + let roots = ([URL(fileURLWithPath: "/Applications")] + + FileManager.default.urls(for: .applicationDirectory, in: .userDomainMask)) + .map { $0.resolvingSymlinksInPath().standardizedFileURL.path } + // Anywhere *under* an Applications directory, not only directly in one. + // Comparing just the immediate parent excluded + // /Applications/Utilities/Dezhban.app — an ordinary thing for someone to do + // with an app, and certainly a location it is going to stay in. That user + // never migrated, so the legacy item kept starting the app with no marker + // and "Open minimized" stayed broken for them permanently, reported only in + // a log line nobody reads. + return roots.contains { bundle == $0 || bundle.hasPrefix($0 + "/") } + } + + /// Which way the user was moving the switch. + /// + /// The live state alone is not enough: a registered agent means "on, as asked" + /// to somebody enabling and "the unregister failed" to somebody disabling, and + /// those need different outcomes. Deriving without it reported `.enabled` — + /// "App will open at login." — to a user who had just clicked *off*, which made + /// `.agentStuck` unreachable from `disable()` whenever a legacy registration + /// also survived. `.agentStuck` is the only outcome carrying the line telling + /// them to clear it in System Settings, and the only non-transient one, so the + /// message was then wiped by the next refresh and the switch sat back ON with no + /// explanation. + enum Direction { case enabling, disabling } + + /// The truthful outcome for whatever is live right now. + /// + /// Both "the legacy item survived" branches used to answer from their own + /// branch — `legacyEnabled ? .legacyStuck : .blockedByLegacy` — which ignored + /// the agent. With a dormant `.requiresApproval` legacy item that will not + /// retract AND a live agent registration, that returned `.blockedByLegacy`: + /// `isOn == false`, a message asserting "it has been left off", and a switch + /// snapping OFF while `isEnabled` said true and the next `seed()` flipped it + /// back. Deriving from the live state instead makes `isOn` agree with + /// `isEnabled` by construction, which is what `isEnabled`'s docstring demands. + /// + /// `fallback` is used only when nothing is live at all. + private static func liveOutcome(_ direction: Direction, fallback: Outcome) -> Outcome { + if legacyEnabled { return .legacyStuck } + let agent = service.status + switch direction { + case .enabling: + // No `.agentStuck` here: it tells the user to go and *remove* the login + // item, which is the opposite of what somebody enabling asked for. The + // two registered statuses are both answered above it, so it was only ever + // reachable by the status changing between two of the three reads this + // used to take. + if agent == .enabled { return .enabled } + if agent == .requiresApproval { return .awaitingApproval } + case .disabling: + // Anything still registered after a disable is a retraction that failed, + // whatever status it wears. `.awaitingApproval` in particular would tell + // someone who clicked *off* to go and enable Dezhban. + if isRegistered(agent) { return .agentStuck } + } + return fallback + } + + /// Words, not a raw `SMAppService.Status`. It is an imported `NS_ENUM` with no + /// `CustomStringConvertible`, so interpolating it put + /// `SMAppService.Status(rawValue: 3)` in front of the user — in the very type + /// that exists so the UI can say something true. + private static func describe(_ status: SMAppService.Status) -> String { + switch status { + case .notRegistered: return "macOS did not keep the registration." + case .enabled: return "the login item is enabled." + case .requiresApproval: + return "macOS needs you to approve Dezhban in System Settings → General → Login Items." + case .notFound: + return "macOS could not find the login item inside the app bundle — reinstall Dezhban." + @unknown default: return "macOS reported an unrecognised login-item state." + } + } + + /// Whether `target` is still registered, giving the status a moment to catch up. + /// + /// `SMAppService.status` is not documented to update synchronously with + /// `unregister()`, and a single read taken microseconds afterwards is what every + /// "the retraction failed" report rests on. If it lags, `disable()` snaps the + /// switch back ON with "macOS would not remove the login item" over a retraction + /// that succeeded, and the uninstall errand exits non-zero so the script prints + /// its refusal warning over a clean removal — the crying-wolf failure the + /// `.notFound`-means-"cannot tell" predicate was deleted for, reached from the + /// other side. + /// + /// Bounded to ~150ms and only ever on the way to reporting a *refusal*, which is + /// the rare path; a success is returned on the first read. This runs on the + /// mutation queue, never the main thread. + private static func stillRegistered(_ target: SMAppService) -> Bool { + for _ in 0 ..< 3 { + if !registered(target) { return false } + usleep(50_000) + } + return registered(target) + } + + /// The agent's status once it has had a moment to catch up with a `register()`. + /// + /// The mirror of `stillRegistered`, for the other direction, and it was missing: + /// the register tail took three separate status reads with no settle, so a status + /// that had not yet caught up gave `.failed` — `isOn == false` — and the switch + /// snapped OFF saying "macOS did not keep the registration" while the agent *was* + /// registered and the app would start at login. That is the same switch-versus- + /// reality lie `stillRegistered` was introduced to kill, on the register side. + /// Three reads also let the status change *between* them, which produced the + /// self-contradicting "Could not change the login item: the login item is + /// enabled." + /// + /// Returns on the first read that shows a registration; only the failing path + /// waits, and never on the main thread. + private static func settledStatus() -> SMAppService.Status { + for _ in 0 ..< 3 { + let status = service.status + if isRegistered(status) { return status } + usleep(50_000) + } + return service.status + } + + private static func unregister(_ target: SMAppService, what: String) { + do { + try target.unregister() } catch { - NSLog("DezhbanMenu: login item toggle failed: \(error)") + NSLog("DezhbanMenu: could not unregister the \(what): \(error)") } - return isEnabled } } diff --git a/gui/macos/Sources/DezhbanMenu/MainWindow.swift b/gui/macos/Sources/DezhbanMenu/MainWindow.swift index 4bc23ab..d4b49a5 100644 --- a/gui/macos/Sources/DezhbanMenu/MainWindow.swift +++ b/gui/macos/Sources/DezhbanMenu/MainWindow.swift @@ -68,6 +68,12 @@ final class MainWindow: NSObject, NSWindowDelegate { styleMask: [.titled, .closable, .miniaturizable, .resizable, .fullSizeContentView], backing: .buffered, defer: false) win.isReleasedWhenClosed = false + // AppKit state restoration would otherwise reopen this window at launch + // on its own, entirely outside the "Open minimized" check in + // AppDelegate — which is half of why that setting appeared not to work. + // Frame and sidebar position still persist; those go through + // setFrameAutosaveName and the split view's autosave, not restoration. + win.isRestorable = false win.delegate = self // 820 wide, not 640: the Help pane's inner HSplitView needs 620pt of // detail (200 sidebar + 420 page) and could not fit at the old minimum diff --git a/gui/macos/Sources/DezhbanMenu/SettingsView.swift b/gui/macos/Sources/DezhbanMenu/SettingsView.swift index 63d6764..d3c02fb 100644 --- a/gui/macos/Sources/DezhbanMenu/SettingsView.swift +++ b/gui/macos/Sources/DezhbanMenu/SettingsView.swift @@ -18,6 +18,53 @@ struct SettingsView: View { @EnvironmentObject var state: AppState @State private var loginEnabled = false + /// Bumped by every login-item click, so an asynchronous status read that was + /// already in flight cannot land afterwards and overwrite the result. + /// + /// Both readers of `LoginItem` are off the main thread now, and `seed()` runs + /// on every `didBecomeActiveNotification` — which macOS delivers *during* a + /// login-item change, since it surfaces System Settings or an approval prompt. + /// So the ordering was really available: click off, `seed()` starts a read that + /// still sees the registration, the click's own completion writes `false`, then + /// the stale read writes `true`. The switch then reads ON with nothing starting + /// the app at login until the next activation. + @State private var loginRevision = 0 + /// True while a login-item change is in flight. + /// + /// The revision alone was not enough: `seed()` captures the current revision + /// without bumping it, so a status read *started after* a click shares that + /// click's revision and passes the equality check. This is the other half — + /// while a mutation is outstanding, no read may write the switch, whichever + /// order the two happen to complete in. + @State private var loginPending = false + /// The login item's own result line. + /// + /// Separate from the pane's shared `status` because the two are different facts + /// with different lifetimes: `seed()` rewrites `status` on every + /// `didBecomeActiveNotification` — which macOS delivers *during* a login-item + /// change, since it surfaces System Settings or an approval prompt — and the + /// service toggle owns it for the length of a privileged sequence. Sharing the + /// line meant the `awaitingApproval` and `legacyStuck` guidance, the entire + /// reason `Outcome` carries a message, could be wiped before it was read; and + /// guarding it in one direction only moved the loss to the other. + @State private var loginMessage: String? + /// True while `loginMessage` explains a refusal the user has to act on. + /// + /// A refresh may clear a message about a moment that has passed; it must not + /// clear one that is the only account of why a click did not take. See + /// `LoginItem.Outcome.isTransient`. + @State private var loginMessageIsTransient = true + /// What the switch read when `loginMessage` was written. + /// + /// A message that explains a refusal has to survive a refresh, but not forever: + /// once the user clears the condition in System Settings and comes back, the + /// switch moves and the refusal is false. Without this, that text stayed on + /// screen contradicting the switch, clearable only by another click. + @State private var loginMessageForEnabled: Bool? + /// True while `loginMessage` is waiting on the user approving Dezhban in System + /// Settings — a condition the switch cannot show, so only the status changing + /// may clear it. See `LoginItem.Outcome.awaitsApproval`. + @State private var loginMessageAwaitsApproval = false @State private var notifyPrefs = NotificationManager.prefs @State private var checkUpdatesEnabled = true @State private var launchVisibility: LaunchVisibility = .bootOnly @@ -168,6 +215,19 @@ struct SettingsView: View { Toggle("Open this app at login", isOn: loginBinding) .help("Registers the app as a login item (System Settings → General → Login Items). " + "This is only the status display — the guard itself is the system service above.") + // Its own line, not the pane's shared `status`. The login item is + // the one control here whose result can be a several-second + // round-trip AND can need the user to go and do something in + // System Settings, so it was competing with the service toggle's + // progress message for a single line: each clobbered the other, + // and a guard against one direction only moved the loss to the + // other. Two facts, two lines. + if let loginMessage { + Text(loginMessage) + .font(.caption) + .foregroundStyle(.secondary) + .fixedSize(horizontal: false, vertical: true) + } Picker("Open minimized", selection: launchVisibilityBinding) { ForEach(LaunchVisibility.allCases) { choice in Text(choice.label).tag(choice) @@ -780,11 +840,74 @@ struct SettingsView: View { private var loginBinding: Binding { Binding( get: { loginEnabled }, - set: { _ in - loginEnabled = LoginItem.toggle() - status = loginEnabled - ? "App will open at login." - : "App will not open at login." + set: { wanted in + // `wanted`, not a re-read of live state: the switch can be stale + // (the login item is also removable in System Settings), and + // deciding from a fresh read then inverted the click. + // + // The outcome, not a bool: macOS can accept the registration and + // still hold it for the user's approval, and there is one path + // where only they can clear the old login item. A switch that + // snaps back with no explanation reads as a bug. + // + // Off the main thread: `set(enabled:)` is six to eight blocking + // SMAppService round-trips over XPC plus an unregister, and this + // runs from a SwiftUI setter, so doing it inline beachballs the + // Settings window. It is the same cost that had the migration moved + // off-main in AppDelegate. The switch moves immediately to where the + // user put it and is corrected from the outcome when it lands — and + // on the disable path launchd may terminate the app partway + // through (ADR-0014's known risk), which is one more reason not to + // be holding the main thread while it happens. + loginRevision += 1 + let revision = loginRevision + loginPending = true + loginEnabled = wanted + loginMessage = wanted ? "Registering the login item…" : "Removing the login item…" + // A progress line is transient by nature. Left inheriting the + // previous outcome's value, a refusal's `false` made it un-clearable + // if this click's completion was then superseded. + loginMessageIsTransient = true + loginMessageForEnabled = wanted + loginMessageAwaitsApproval = false + // The enqueueing form, so two quick clicks are applied in the order + // they were made — dispatching each to a concurrent queue let them + // race into LoginItem's serial queue and land out of order. + LoginItem.set(enabled: wanted) { outcome in + // A newer click supersedes this one's result. + guard revision == loginRevision else { return } + loginPending = false + // Only an outcome that describes the switch's own state may move + // it. `.unstableLocation` refuses the click and changes nothing, + // so asserting its `isOn == false` painted the switch OFF while + // login-at-launch was still live through the installed copy — + // the switch reading the opposite of reality, which is the one + // thing `Outcome` exists to prevent. + if outcome.describesSwitchState { loginEnabled = outcome.isOn } + // Written unconditionally: this is the login item's own line, + // so there is nothing else to collide with. That is the point of + // having split it from the pane's shared status. + loginMessage = outcome.message + loginMessageIsTransient = outcome.isTransient + // Only for outcomes that describe the switch's own state. A + // refusal that has nothing to do with it — this copy may not + // touch the login item at all — is not invalidated by the switch + // moving, and tying it to `isOn == false` had it wiped on the + // next activation: a dev build beside an installed copy reads the + // switch ON permanently, so the disagreement never resolves and + // the explanation went every time, leaving the unexplained + // snap-back the message exists to prevent. + loginMessageForEnabled = outcome.describesSwitchState ? outcome.isOn : nil + loginMessageAwaitsApproval = outcome.awaitsApproval + // And bumped, so any status read that was already in flight + // cannot land afterwards and clear this. `loginPending` cannot + // cover it: seed()'s read is a queue.sync behind this very + // mutation, so it is *guaranteed* to complete after it, by which + // time pending is already false — and it then nils the one line + // that tells the user to go to System Settings, a moment after it + // appeared. + loginRevision += 1 + } }) } @@ -909,7 +1032,56 @@ struct SettingsView: View { refreshTokenCapability() status = "Loading…" canApply = false - loginEnabled = LoginItem.isEnabled + // Off-main for the same reason `set(enabled:)` is: this is two blocking + // SMAppService status reads over XPC (it was one before the agent), and + // opening the Settings pane should not hitch on them. Stamped with the + // click revision so a read started before a click cannot land after it — + // see `loginRevision`. + let revision = loginRevision + DispatchQueue.global(qos: .userInitiated).async { + let live = LoginItem.state + DispatchQueue.main.async { + guard revision == loginRevision, !loginPending else { return } + loginEnabled = live.enabled + // Clear only a message about a moment that has passed. "macOS is + // holding this for your approval" outlived the approval — the user + // went to System Settings, granted it, came back, which is what + // fires this seed — and a fresh status read makes the switch speak + // for itself there. + // + // But not the ones explaining a refusal. Clicking away from the app + // and back is enough to fire this, so clearing unconditionally erased + // "Dezhban has to live in Applications to open at login" a moment + // after the switch snapped back, leaving exactly the unexplained + // snap-back this message exists to prevent. + // Three ways a message stops being worth showing: it described a + // moment that has passed; it was waiting on an approval that has now + // been granted (or the registration is gone) — which the switch + // cannot show, so only this can clear it; or the switch has since + // moved away from the state it was written about. + let approvalSettled = loginMessageAwaitsApproval && !live.awaitingApproval + let switchMoved = !loginMessageAwaitsApproval + && (loginMessageForEnabled.map { $0 != live.enabled } ?? false) + if loginMessageIsTransient || approvalSettled || switchMoved { + loginMessage = nil + loginMessageForEnabled = nil + loginMessageAwaitsApproval = false + } + // And *state* it, not only preserve it. This read was only ever + // consulted to decide whether to clear a message a click had left + // behind — so opening the pane with no preceding click, on an install + // whose agent the user had switched off under Login Items, painted the + // switch ON with nothing said, while nothing started the app at login. + // `enabled` counts an awaiting-approval registration, so the switch + // cannot express it; this is the only thing that can. + if live.awaitingApproval, loginMessage == nil { + loginMessage = LoginItem.Outcome.awaitingApproval.message + loginMessageIsTransient = false + loginMessageAwaitsApproval = true + loginMessageForEnabled = live.enabled + } + } + } notifyPrefs = NotificationManager.prefs checkUpdatesEnabled = UpdateChecker.isEnabled launchVisibility = LaunchPreference.current diff --git a/gui/macos/Sources/DezhbanMenu/main.swift b/gui/macos/Sources/DezhbanMenu/main.swift index 99be59e..97f3a53 100644 --- a/gui/macos/Sources/DezhbanMenu/main.swift +++ b/gui/macos/Sources/DezhbanMenu/main.swift @@ -1,4 +1,5 @@ import AppKit +import DezhbanCore /// A minimal programmatic main menu. Without one, the SwiftUI main window's /// text fields have no Edit menu (no ⌘C/⌘V/⌘X/⌘A) and ⌘W/⌘Q do nothing while @@ -51,6 +52,215 @@ func makeMainMenu() -> NSMenu { return main } +/// The hand-off request beside this install's session lock, once known. +/// +/// A global because both ends of the launch need it: `acquireSessionOwnership()` +/// writes it from a losing copy, and `AppDelegate` claims it — from the +/// notification handler, and from a bounded backstop after launch, which is what +/// makes a request that arrived before the observer existed still get honoured. +var sessionHandoff: HandoffRequest? + +/// Whether this process actually holds the session lock. +/// +/// False on the `.unavailable` path, where the app deliberately starts anyway +/// rather than refuse to launch over a broken support directory. Such a copy is +/// running alongside the real owner, and must not also answer hand-offs: both would +/// open a window for one double-click, each with its own debounce, so the debounce +/// cannot suppress the second. +var sessionOwnsLock = false + +/// Retracts every login registration and exits, without starting the app. +/// +/// The uninstaller needs this. A LaunchServices login item disappeared with its +/// bundle; the launchd agent that replaced it does not — and `launchctl bootout` +/// only unloads the job for this boot, leaving the registration that created it +/// to reload at the next login against a plist inside a bundle that has been +/// deleted. Only `SMAppService.unregister()` actually retracts it, and only the +/// app can call it, so `packaging/macos/uninstall.sh` runs the binary this way +/// (as the console user, in their GUI session) before deleting anything. +/// +/// Handled before the session lock, deliberately: this is not a second copy of +/// the app competing for the session, it is a one-shot errand, and it must work +/// while the app is running — which is exactly when the uninstaller finds it. +func retractLoginRegistrationsAndExit() { + // The exit status is the whole channel. `unregister()` only logs its throw and + // the uninstaller discards this process's output, so a refusal used to leave + // the Login Items entry behind — pointing at a bundle about to be deleted, and + // unreachable afterwards — while the script printed "service unregistered, + // files deleted". + exit(LoginItem.retractAll() ? 0 : 1) +} + +/// Exits if another copy of this install already owns the session. +/// +/// The login item is a launchd agent now, and `register()` on a `RunAtLoad` job +/// `exec`s the binary immediately — from the Settings toggle and from the +/// migration, both of which run while the app is up. launchd does not go through +/// LaunchServices, so nothing else dedupes it. Without this the user gets two +/// menubar items, and the duplicate carries `--background`, so under the default +/// "Only at login" it opens no window and there is no way to tell which icon is +/// which. See `SessionLock` and docs/adr/0014-login-item-launch-marker.md. +/// +/// Returns the lock on success. The caller must keep it alive for the lifetime of +/// the process — the lock IS the open file descriptor. +func acquireSessionOwnership() -> SessionLock? { + // No bundle identifier means a bare `swift run` binary: no agent could have + // spawned it, and nothing to scope a lock to. + guard let id = Bundle.main.bundleIdentifier, + let support = FileManager.default + .urls(for: .applicationSupportDirectory, in: .userDomainMask).first + else { return nil } + + let lock = SessionLock.forBundle( + path: Bundle.main.bundleURL.path, identifier: id, supportDirectory: support) + sessionHandoff = HandoffRequest.beside(lock: lock.url) + switch lock.acquire() { + case .acquired: + sessionOwnsLock = true + // Anything already on disk was meant for a predecessor, not for us — and so + // is any `.claiming-*` file a predecessor died in the middle of. + sessionHandoff?.discard() + sessionHandoff?.sweepAbandonedClaims() + return lock + case .unavailable(let why): + // Never refuse to start over this. A duplicate icon is a smaller failure + // than an app that will not launch because a support directory is broken. + NSLog("DezhbanMenu: session lock unavailable, starting anyway: \(why)") + // No discard here. `discard()` is for a process that has just *taken* the + // lock, on the grounds that anything on disk was meant for a predecessor — + // and this process took nothing. A transient open() failure in a third + // launch would otherwise delete a request the real session owner was about + // to claim, losing that user's double-click. + return lock + case .heldByAnother: + // Somebody holds the lock — but only a *live* somebody may be handed the + // launch. `flock` is released by the kernel when its holder dies, so locally + // "held" implies "alive"; over a network home, where the server emulates it, + // an advisory lock can outlive the process that took it. Every launch then + // finds the lock taken, posts a hand-off nobody will ever claim, and exits — + // the app becomes permanently unstartable with nothing said, which is worse + // than the duplicate icon this guard exists to avoid, and is the opposite of + // how every other failure here degrades ("refusing to start because a support + // directory is broken would be a worse bug"). + // + // Decided on where the lock lives, not on a timer. Waiting for the incumbent + // to appear in LaunchServices was the wrong shape: at login it is + // launchd-exec'd and often not registered yet, so a bounded probe times out + // on a *live* incumbent — and then started a second copy of the app, the + // failure this lock exists to prevent, on the common path. It also discarded + // the hand-off it had just posted, which the `.unavailable` case thirty lines + // up refuses to do for exactly the reason stated there. + // + // Locally, `flock` is the kernel's: held means alive, so this yields whether + // or not anyone has shown up in LaunchServices yet, and the request it posts + // is claimed by the incumbent's own backstop. Only on a network home, where + // the lock can outlive its holder, may "nobody there" mean the lock is stale. + let mePID = ProcessInfo.processInfo.processIdentifier + let ownBundle = Bundle.main.bundleURL.resolvingSymlinksInPath().standardizedFileURL + let incumbent = NSRunningApplication + .runningApplications(withBundleIdentifier: id) + .first { + $0.processIdentifier != mePID && !$0.isTerminated + && $0.bundleURL?.resolvingSymlinksInPath().standardizedFileURL == ownBundle + } + if incumbent == nil, !lock.isOnLocalVolume { + NSLog("DezhbanMenu: the session lock is held on a network home but no live copy " + + "of this install owns it; treating it as stale and starting anyway") + // Not discarded: a request on disk may belong to a launch the real owner + // is about to answer, and this process took nothing. + // + // Owner enough to answer hand-offs, though. Without this the app started, + // became the incumbent, and then every later launch found it, posted a + // request and exited while nobody was listening — the silent no-op again, + // permanently, for that install. + sessionOwnsLock = true + sessionHandoff?.sweepAbandonedClaims() + return lock + } + // A background launch loses silently — that copy was never going to show + // the user anything. A launch the user performed must not be a no-op, so + // hand them over to the instance that owns the session. + if !LaunchVisibility.isBackgroundLaunch(arguments: CommandLine.arguments) { + incumbent?.activate() + // Ask it to open its window — which it may not currently have, since + // the incumbent may be a --background login launch. + // + // NOT gated on "Open minimized". It was, on the reasoning that + // "Always" has to mean always — but the preference governs the + // *launch*, and a user-initiated launch of an already-running app is + // not one: once the incumbent has finished starting, LaunchServices + // turns the same double-click into a reopen, and + // `applicationShouldHandleReopen` opens the window unconditionally in + // every mode, by design ("the Dock icon and Open Dezhban… open the + // window regardless"). So gating here bought no consistency at all — + // it gave the same gesture opposite answers depending on whether the + // incumbent happened to have finished starting yet — and cost the one + // launch that had no other route to a window. + // + // A notification rather than re-opening the bundle through + // NSWorkspace: asking LaunchServices to open the app we are in the + // middle of quitting could spawn yet another copy, which would find + // the lock held and ask again. + // + // Scoped to this install by posting the bundle PATH as the object. + // The name derives from the bundle id, and the lock deliberately lets + // dist/Dezhban.app run beside an installed copy — an unscoped + // notification would have a duplicate launch of one install open the + // other install's window. + // The file first, then the notification. The notification is the fast + // path but is never queued, and the incumbent may still be starting up + // with no observer installed — the file is the one that waits, and the + // incumbent's launch-time backstop finds it. Whichever of the two gets + // there claims it, so the window opens once (see HandoffRequest). + // One token for both signals, so the incumbent can recognise the second + // one as describing a request it has already answered — and a genuinely + // new launch, with a new token, as a new request. + let token = UUID().uuidString + // Defaults to NOT landed. A nil `sessionHandoff` is unreachable today + // (it is assigned immediately above the acquire) but the safe reading of + // "there was no file to write" is the same as a failed write: say so, so + // the notification carries the fileless marker. Defaulting to success + // meant a future reordering would drop a launch with nothing logged. + var fileLanded = false + switch sessionHandoff?.post(token: token) { + case .success: + fileLanded = true + case .failure(let error): + NSLog("DezhbanMenu: could not record the hand-off request: \(error)") + case nil: + NSLog("DezhbanMenu: no hand-off request path; relying on the notification") + } + // Whether the file landed travels WITH the notification, because this + // process is the only one that knows. The incumbent requires a file + // outside its launch-time backstop window — that is what keeps an + // unauthenticated notification from being an activate-on-demand channel + // — so without this, a failed write (read-only or full home, wrong + // permissions on the support directory: the same conditions the session + // lock is written to tolerate) turned a launch the notification alone + // used to handle into a visible no-op, which is the one outcome this + // whole mechanism exists to prevent. + DistributedNotificationCenter.default().postNotificationName( + NSNotification.Name(AppDelegate.openWindowNotification), + object: Bundle.main.bundleURL.resolvingSymlinksInPath().standardizedFileURL.path, + userInfo: fileLanded + ? [AppDelegate.handoffTokenKey: token] + : [AppDelegate.handoffTokenKey: token, AppDelegate.handoffFilelessKey: "1"], + deliverImmediately: true) + } + NSLog("DezhbanMenu: another copy of this install owns the session; exiting") + exit(0) + } +} + +// Both of these run before NSApplication exists: the errand mode never becomes +// an app at all, and a duplicate must exit before it can put a tile in the Dock. +if CommandLine.arguments.contains("--unregister-login-item") { + retractLoginRegistrationsAndExit() +} +// Held for the lifetime of the process — the lock is the open file descriptor, +// so letting this go out of scope would release the session to the next starter. +let sessionLock = acquireSessionOwnership() + // Regular app (not an LSUIElement agent): the Dock tile doubles as a state // display — AppDelegate swaps NSApp.applicationIconImage to match the // enforcement posture, and that needs a Dock icon to exist. The bundled diff --git a/gui/macos/Tests/DezhbanCoreTests/HandoffRequestTests.swift b/gui/macos/Tests/DezhbanCoreTests/HandoffRequestTests.swift new file mode 100644 index 0000000..4df36a1 --- /dev/null +++ b/gui/macos/Tests/DezhbanCoreTests/HandoffRequestTests.swift @@ -0,0 +1,136 @@ +import Foundation +import Testing +@testable import DezhbanCore + +struct HandoffRequestTests { + private func tempDir() throws -> URL { + let dir = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("dezhban-handoff-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + return dir + } + + /// The whole point: a request written while nobody was observing is still + /// there to be found — and found once. + @Test func aPostedRequestIsClaimedOnce() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let request = HandoffRequest(url: dir.appendingPathComponent("a.handoff")) + + request.post(token: "t1") + #expect(request.claim() == .fresh(token: "t1")) + #expect(request.claim() == .absent) + } + + /// Nothing waiting means nothing to do, which is the ordinary case every time + /// the backstop looks. Distinct from `.lost`, which means somebody else is + /// already acting on one. + @Test func noRequestIsNotARequest() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + #expect(HandoffRequest(url: dir.appendingPathComponent("b.handoff")).claim() == .absent) + } + + /// `discard()` is what a process that has just taken the lock calls: whatever + /// is on disk was meant for its predecessor. + @Test func discardRemovesWithoutReporting() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let request = HandoffRequest(url: dir.appendingPathComponent("e.handoff")) + + request.post(token: "t1") + request.discard() + #expect(request.claim() == .absent) + } + + /// The token is what lets the two signals for one request be told apart from two + /// requests. Timing could not, which is why three attempts at a debounce each + /// both duplicated a window and swallowed a real launch. + @Test func theClaimCarriesThePostedToken() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let request = HandoffRequest(url: dir.appendingPathComponent("i.handoff")) + + request.post(token: "abc-123") + #expect(request.claim() == .fresh(token: "abc-123")) + + // A request from an older build carries nothing; `nil` means "cannot dedupe", + // which the caller treats as its own identity rather than as a match. + try Data().write(to: request.url) + #expect(request.claim() == .fresh(token: nil)) + } + + /// Two claimers overlapping on one request — the notification handler and the + /// launch-time backstop, which is the pair this type exists to arbitrate. The + /// loser must be told it lost, not that there was nothing there: only that + /// distinction stops it opening a second window half a second after the first, + /// or reopening one the user has just closed. + @Test func anOverlappingClaimerIsToldItLost() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let request = HandoffRequest(url: dir.appendingPathComponent("f.handoff")) + + request.post(token: "t1") + // Stands in for the other claimer winning between this one's stat and its + // remove — the only way `.lost` can arise, and the reason `claim` takes + // the hook. + let claim = request.claim(interleaved: { request.discard() }) + #expect(claim == .lost) + } + + /// A request that cannot be removed must not look like one somebody else took. + /// Folded together, a permanent failure made every claimer stand down forever + /// and killed the mechanism silently. + @Test func anUnremovableRequestIsBlockedNotLost() throws { + // Root ignores directory permissions, so the mechanism this test uses to + // make a file unremovable does not work for it — the unlink succeeds, + // `claim()` returns `.fresh`, and the test would fail for a reason that has + // nothing to do with the code. testing.md routinely asks contributors to run + // privileged checks on this machine, so skip rather than lie. + try #require(getuid() != 0, "cannot make a file unremovable for root") + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let request = HandoffRequest(url: dir.appendingPathComponent("h.handoff")) + request.post(token: "t1") + + // Read-only parent: the file is visible but cannot be unlinked. + try FileManager.default.setAttributes([.posixPermissions: 0o500], ofItemAtPath: dir.path) + defer { try? FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: dir.path) } + + guard case .blocked = request.claim() else { + Issue.record("expected .blocked for an unremovable request") + return + } + } + + /// One install's sweep must not touch another's in-flight claim. The support + /// directory is shared by every copy of the app on purpose, so a directory-wide + /// sweep silently broke the other copy's dedupe. + @Test func sweepingLeavesAnotherRequestsClaimsAlone() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let mine = HandoffRequest(url: dir.appendingPathComponent("instance-aaa.handoff")) + let theirs = HandoffRequest(url: dir.appendingPathComponent("instance-bbb.handoff")) + + // Stand in for a claim each install has renamed aside but not yet read. + let myClaim = dir.appendingPathComponent( + HandoffRequest.claimPrefix(for: mine.url) + "1") + let theirClaim = dir.appendingPathComponent( + HandoffRequest.claimPrefix(for: theirs.url) + "1") + try Data().write(to: myClaim) + try Data().write(to: theirClaim) + + mine.sweepAbandonedClaims() + #expect(!FileManager.default.fileExists(atPath: myClaim.path)) + #expect(FileManager.default.fileExists(atPath: theirClaim.path)) + } + + /// Scoped per install, like the lock it sits beside — two installs may + /// legitimately run side by side. + @Test func theRequestSitsBesideItsOwnLock() { + let one = HandoffRequest.beside(lock: URL(fileURLWithPath: "/x/instance-aaa.lock")) + let two = HandoffRequest.beside(lock: URL(fileURLWithPath: "/x/instance-bbb.lock")) + #expect(one.url != two.url) + #expect(one.url.path == "/x/instance-aaa.handoff") + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/LaunchVisibilityTests.swift b/gui/macos/Tests/DezhbanCoreTests/LaunchVisibilityTests.swift index 3cbfc88..d842186 100644 --- a/gui/macos/Tests/DezhbanCoreTests/LaunchVisibilityTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/LaunchVisibilityTests.swift @@ -3,20 +3,40 @@ import Testing struct LaunchVisibilityTests { /// The default must reproduce what the app did before the setting existed: - /// open on a deliberate launch, stay hidden when macOS started us at login. + /// open when the user starts it, stay hidden when macOS started us at login. /// An upgrade is a no-op for anyone who never touches it. @Test func bootOnlyIsTheOldUnconditionalBehaviour() { - #expect(LaunchVisibility.bootOnly.opensWindow(deliberateLaunch: true)) - #expect(!LaunchVisibility.bootOnly.opensWindow(deliberateLaunch: false)) + #expect(LaunchVisibility.bootOnly.opensWindow(backgroundLaunch: false)) + #expect(!LaunchVisibility.bootOnly.opensWindow(backgroundLaunch: true)) } @Test func neverAndAlwaysIgnoreHowTheLaunchHappened() { - for deliberate in [true, false] { - #expect(LaunchVisibility.never.opensWindow(deliberateLaunch: deliberate)) - #expect(!LaunchVisibility.always.opensWindow(deliberateLaunch: deliberate)) + for background in [true, false] { + #expect(LaunchVisibility.never.opensWindow(backgroundLaunch: background)) + #expect(!LaunchVisibility.always.opensWindow(backgroundLaunch: background)) } } + /// The marker is the whole mechanism: the login LaunchAgent passes it and + /// nothing else does. A rename here without a matching edit to + /// LoginAgent.plist silently restores the bug this replaced. + @Test func backgroundLaunchIsReadFromTheArgumentTheAgentPasses() { + #expect(LaunchVisibility.backgroundArgument == "--background") + #expect(LaunchVisibility.isBackgroundLaunch( + arguments: ["/Applications/Dezhban.app/Contents/MacOS/DezhbanMenu", "--background"])) + #expect(!LaunchVisibility.isBackgroundLaunch( + arguments: ["/Applications/Dezhban.app/Contents/MacOS/DezhbanMenu"])) + } + + /// An unmarked launch must read as a user launch. The failure mode of + /// guessing wrong in this direction is a window nobody asked for; guessing + /// wrong the other way hides the window from someone who did. + @Test func anEmptyArgumentListIsAUserLaunch() { + #expect(!LaunchVisibility.isBackgroundLaunch(arguments: [])) + #expect(LaunchVisibility.bootOnly.opensWindow( + backgroundLaunch: LaunchVisibility.isBackgroundLaunch(arguments: []))) + } + /// The raw values are persisted in UserDefaults, so renaming one silently /// resets every existing user's choice to the default. @Test func rawValuesArePersistedIdentifiers() { diff --git a/gui/macos/Tests/DezhbanCoreTests/SessionLockTests.swift b/gui/macos/Tests/DezhbanCoreTests/SessionLockTests.swift new file mode 100644 index 0000000..e1f8e05 --- /dev/null +++ b/gui/macos/Tests/DezhbanCoreTests/SessionLockTests.swift @@ -0,0 +1,156 @@ +import Foundation +import Testing +@testable import DezhbanCore + +struct SessionLockTests { + private func tempDir() throws -> URL { + let dir = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("dezhban-instancelock-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + return dir + } + + /// The case that put this code here: registering the login agent execs a + /// second copy while the first is serving the menubar. `flock` is per open + /// file description, so a second `acquire()` on the same path is refused even + /// from within one process — which is exactly what makes this testable. + @Test func aSecondHolderIsRefused() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let path = dir.appendingPathComponent("a.lock") + + let first = SessionLock(url: path) + let second = SessionLock(url: path) + defer { first.release(); second.release() } + + #expect(first.acquire() == .acquired) + #expect(second.acquire() == .heldByAnother) + } + + /// The failure the previous design could not rule out: two copies each + /// deciding the other one wins, leaving no app running at all. A lock cannot + /// express that — someone holds it. + @Test func exactlyOneOfManyContendersWins() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let path = dir.appendingPathComponent("b.lock") + + let contenders = (0 ..< 5).map { _ in SessionLock(url: path) } + defer { contenders.forEach { $0.release() } } + + let winners = contenders.filter { $0.acquire() == .acquired } + #expect(winners.count == 1) + } + + /// Releasing hands the session to the next starter. This is what makes a + /// crashed or force-quit predecessor harmless: the kernel does this for us + /// when the process dies, which a pid file would not. + @Test func releasingLetsTheNextInstanceIn() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let path = dir.appendingPathComponent("c.lock") + + let first = SessionLock(url: path) + let second = SessionLock(url: path) + defer { second.release() } + + #expect(first.acquire() == .acquired) + #expect(second.acquire() == .heldByAnother) + first.release() + #expect(second.acquire() == .acquired) + } + + /// Re-acquiring is not a second holder — a caller that asks twice must not be + /// told it lost to itself. + @Test func reacquiringTheSameLockIsIdempotent() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let lock = SessionLock(url: dir.appendingPathComponent("d.lock")) + defer { lock.release() } + + #expect(lock.acquire() == .acquired) + #expect(lock.acquire() == .acquired) + } + + /// An unwritable location must not stop the app from starting. A broken + /// support directory is a worse thing to fail a launch on than a duplicate + /// icon. + @Test func anUnopenableLockPathIsReportedRatherThanBlocking() { + let lock = SessionLock(url: URL(fileURLWithPath: "/dev/null/nope/e.lock")) + defer { lock.release() } + guard case .unavailable = lock.acquire() else { + Issue.record("expected .unavailable for an unopenable path") + return + } + } + + /// Two installs of the same app are not duplicates of each other: the + /// documented GUI dev loop runs dist/Dezhban.app while /Applications holds a + /// released copy, and an identifier-scoped lock made the dev build exit on + /// launch and silently test the installed one. + @Test func differentInstallPathsGetDifferentLocks() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let installed = SessionLock.forBundle( + path: "/Applications/Dezhban.app", identifier: "com.example.app", supportDirectory: dir) + let built = SessionLock.forBundle( + path: "/Users/x/dev/dezhban/dist/Dezhban.app", identifier: "com.example.app", + supportDirectory: dir) + defer { installed.release(); built.release() } + + #expect(installed.url != built.url) + #expect(installed.acquire() == .acquired) + #expect(built.acquire() == .acquired) + } + + /// Two spellings of one install are one install. A symlinked install + /// directory is the realistic case (`/tmp` is itself a symlink to + /// `/private/tmp` on macOS), and deriving two locks from it would let both + /// copies run — silently, which is the whole failure this class prevents. + /// + /// Built on a real symlink to a real bundle directory on purpose: + /// `resolvingSymlinksInPath()` returns the path unchanged when the leaf does + /// not exist, so a test written against two invented paths would pass or fail + /// for reasons that have nothing to do with the code. + @Test func equivalentPathsGetTheSameLock() throws { + let root = try tempDir() + defer { try? FileManager.default.removeItem(at: root) } + let locks = root.appendingPathComponent("locks", isDirectory: true) + try FileManager.default.createDirectory(at: locks, withIntermediateDirectories: true) + + let real = root.appendingPathComponent("real", isDirectory: true) + let bundle = real.appendingPathComponent("Dezhban.app", isDirectory: true) + try FileManager.default.createDirectory(at: bundle, withIntermediateDirectories: true) + let link = root.appendingPathComponent("link", isDirectory: true) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: real) + + let viaReal = SessionLock.forBundle( + path: bundle.path, identifier: "com.example.app", supportDirectory: locks) + let viaLink = SessionLock.forBundle( + path: link.appendingPathComponent("Dezhban.app").path, + identifier: "com.example.app", supportDirectory: locks) + defer { viaReal.release(); viaLink.release() } + + #expect(viaReal.url == viaLink.url) + #expect(viaReal.acquire() == .acquired) + #expect(viaLink.acquire() == .heldByAnother) + } + + /// The same install must derive the same name in every process, so the hash + /// cannot be Swift's per-process-seeded one. Pinning a literal is the only + /// way this test can fail if someone swaps it for `hashValue`. + @Test func theLockNameIsStableAcrossProcesses() throws { + let dir = try tempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let a = SessionLock.forBundle( + path: "/Applications/Dezhban.app", identifier: "com.example.app", supportDirectory: dir) + let b = SessionLock.forBundle( + path: "/Applications/Dezhban.app", identifier: "com.example.app", supportDirectory: dir) + #expect(a.url == b.url) + // FNV-1a of the empty string is its offset basis; a seeded hash would not + // reproduce it. + #expect(SessionLock.fnv1a("") == 0xcbf2_9ce4_8422_2325) + #expect(SessionLock.fnv1a("a") == SessionLock.fnv1a("a")) + #expect(SessionLock.fnv1a("a") != SessionLock.fnv1a("b")) + } +} diff --git a/gui/macos/build-app.sh b/gui/macos/build-app.sh index 457ab45..f0be54a 100755 --- a/gui/macos/build-app.sh +++ b/gui/macos/build-app.sh @@ -72,6 +72,91 @@ install -m 0755 "$HERE/askpass.sh" "$APP/Contents/Resources/askpass.sh" # ad-hoc codesign below, so the seal covers it. install -m 0644 "$REPO_ROOT/LICENSE" "$APP/Contents/Resources/LICENSE" +# Login-item LaunchAgent. SMAppService.agent(plistName:) reads it from exactly +# this directory, and it is the only thing that tells a login launch apart from +# a user launch: it passes --background, which LaunchVisibility keys on. A +# bundle assembled without it registers nothing and the app silently stops +# starting at login, so its absence is a build failure rather than a note. +# See docs/adr/0014-login-item-launch-marker.md. +AGENT_LABEL="com.behnam-rk.dezhban.app.login" +AGENT_PLIST="$APP/Contents/Library/LaunchAgents/$AGENT_LABEL.plist" +mkdir -p "$APP/Contents/Library/LaunchAgents" +install -m 0644 "$HERE/LoginAgent.plist" "$AGENT_PLIST" + +# The two facts that make the feature work, asserted rather than commented. +# `install` under `set -e` already aborts if the file does not land, so its +# existence needs no test — but deleting --background from LoginAgent.plist, or +# renaming its Label, passes go test, swift test and this build while silently +# restoring the original bug (or killing login-at-launch outright, reported only +# as an SMAppService status nobody reads). +# +# 1. Label must equal the filename SMAppService.agent(plistName:) is given +# (LoginItem.plistName) — launchd rejects a mismatch. +plist_label="$(plutil -extract Label raw -o - "$AGENT_PLIST")" +if [[ "$plist_label" != "$AGENT_LABEL" ]]; then + echo "build-app.sh: LoginAgent.plist Label is '$plist_label', but it is installed as '$AGENT_LABEL.plist' — launchd would reject the job and the app would not start at login" >&2 + exit 1 +fi +# 2. ProgramArguments must still carry the launch marker LaunchVisibility keys on +# (LaunchVisibility.backgroundArgument), or every login looks like a user +# launch and "Open minimized" silently stops working. +agent_argc="$(plutil -extract ProgramArguments raw -o - "$AGENT_PLIST")" +agent_marker=0 +for ((i = 0; i < agent_argc; i++)); do + if [[ "$(plutil -extract "ProgramArguments.$i" raw -o - "$AGENT_PLIST")" == "--background" ]]; then + agent_marker=1 + fi +done +if [[ "$agent_marker" -ne 1 ]]; then + echo "build-app.sh: LoginAgent.plist ProgramArguments no longer pass --background — a login launch would be indistinguishable from a user launch" >&2 + exit 1 +fi +# 3. The label must be spelled the same in the three places that must agree: +# the plist, LoginItem.plistName (what SMAppService.agent is given), and +# uninstall.sh (what retracts it). Renaming it consistently in the plist AND +# here would otherwise satisfy check 1 while SMAppService named a file that +# does not exist — reported only as the .notFound status nobody reads. +# +# Matched against the exact declaration in each consumer, not merely "the label +# appears somewhere in the file". Two reasons, both learned the hard way: without +# -F the pattern is a regex, so `.` is a wildcard and "…app-login" satisfied a +# check for "…app.login"; and a bare substring search for the label also matched +# LoginItem's dispatch-queue label, "…app.loginitem", which contains it — so the +# check could not fail for that file no matter what plistName said. +swift_decl="private static let plistName = \"$AGENT_LABEL.plist\"" +if ! grep -qF "$swift_decl" "$HERE/Sources/DezhbanMenu/LoginItem.swift"; then + echo "build-app.sh: LoginItem.swift does not declare plistName as '$AGENT_LABEL.plist' — SMAppService would name a plist that does not exist, reported only as the .notFound status nobody reads" >&2 + exit 1 +fi +if ! grep -qxF "LOGIN_AGENT=$AGENT_LABEL" "$REPO_ROOT/packaging/macos/uninstall.sh"; then + echo "build-app.sh: uninstall.sh does not set LOGIN_AGENT to '$AGENT_LABEL' — it would fail to retract the registration it is meant to remove" >&2 + exit 1 +fi +# 4. BundleProgram must name the executable this script actually installs. It is +# bundle-relative and launchd resolves it at load time, so renaming the SwiftPM +# executable target passes go test, swift test and this build while the login job +# fails to load — surfacing only as a status nobody reads. +agent_program="$(plutil -extract BundleProgram raw -o - "$AGENT_PLIST")" +if [[ ! -x "$APP/$agent_program" ]]; then + echo "build-app.sh: LoginAgent.plist BundleProgram is '$agent_program', which is not an executable in the assembled bundle — launchd would fail to load the login job" >&2 + exit 1 +fi +# 5. The bundle identifier has to agree in three places: Info.plist (what macOS +# knows the app as), the agent's AssociatedBundleIdentifiers (what makes the +# Login Items row read "Dezhban" rather than a raw job label), and uninstall.sh's +# APP_BUNDLE_ID (what removes the per-user leftovers, including the flag that +# would otherwise make a later reinstall skip the login-item migration). +bundle_id="$(plutil -extract CFBundleIdentifier raw -o - "$APP/Contents/Info.plist")" +agent_assoc="$(plutil -extract AssociatedBundleIdentifiers.0 raw -o - "$AGENT_PLIST")" +if [[ "$agent_assoc" != "$bundle_id" ]]; then + echo "build-app.sh: LoginAgent.plist AssociatedBundleIdentifiers is '$agent_assoc' but the bundle identifier is '$bundle_id' — the Login Items entry would not be attributed to Dezhban" >&2 + exit 1 +fi +if ! grep -qxF "APP_BUNDLE_ID=$bundle_id" "$REPO_ROOT/packaging/macos/uninstall.sh"; then + echo "build-app.sh: uninstall.sh does not set APP_BUNDLE_ID to '$bundle_id' — it would leave the per-user preferences behind, and a later reinstall would skip the login-item migration" >&2 + exit 1 +fi + # Documentation, rendered from the repo's own markdown into the bundle. Shipping # it means the help pane works with every byte of egress cut — which is exactly # when someone needs it — and that the docs always match the version they diff --git a/packaging/macos/uninstall.sh b/packaging/macos/uninstall.sh index 1361ae5..c7ba454 100755 --- a/packaging/macos/uninstall.sh +++ b/packaging/macos/uninstall.sh @@ -20,6 +20,12 @@ CONFIG_DIR=/etc/dezhban STATE_DIR=/var/db/dezhban PLIST=/Library/LaunchDaemons/dezhban.plist SHARE_DIR=/usr/local/share/dezhban +LOGIN_AGENT=com.behnam-rk.dezhban.app.login +APP_BUNDLE_ID=com.behnam-rk.dezhban.app +LOGIN_ITEM_STUCK=none +# Separate from LOGIN_ITEM_STUCK, which holds one value: the two conditions are +# independent and both have to be reportable at once. +SUPPORT_DIR_KEPT=0 if [ "$(id -u)" -ne 0 ]; then echo "error: run as root — sudo sh $0" >&2 @@ -46,10 +52,319 @@ echo "removing the menubar app ..." # bundle out from under a live process. osascript -e 'tell application "Dezhban" to quit' >/dev/null 2>&1 || true pkill -x DezhbanMenu >/dev/null 2>&1 || true -rm -rf "$APP" + +# The login item is a per-user launchd agent (SMAppService.agent), NOT a +# LaunchServices entry: deleting the bundle does not retract it. Left registered it +# fails to load at every subsequent login and lingers in System Settings → General +# → Login Items as an orphan the user has to hunt down. +# +# Only SMAppService.unregister() actually retracts the registration, and only the +# app can call it — `launchctl bootout` unloads the job for THIS boot and leaves +# the record that recreates it at the next login, pointing at a plist inside the +# bundle we are about to delete. So the app is run one last time, as the console +# user inside their GUI session, purely to retract itself. bootout follows as a +# belt-and-braces unload of the job it just retracted. +# +# All of it needs the user's own launchd session, so root can only reach the +# console user; other accounts get the one command they need, printed at the end. +CONSOLE_USER=$(stat -f %Su /dev/console 2>/dev/null || echo "") +CONSOLE_UID="" +CONSOLE_HOME="" +if [ -n "$CONSOLE_USER" ] && [ "$CONSOLE_USER" != "root" ]; then + CONSOLE_UID=$(id -u "$CONSOLE_USER" 2>/dev/null || echo "") + if [ -z "$CONSOLE_UID" ]; then + # Somebody IS at this Mac; their uid just could not be looked up (an + # unreachable network/AD directory, a damaged local record). Telling them + # "nobody is logged in — re-run from a graphical session" is both false and + # useless advice, since re-running fails the same way. + LOGIN_ITEM_STUCK=no-console-uid + fi + # /Search, not the local node. `dscl .` reads only local records, so for a + # network/LDAP/AD account it returns nothing — leaving the cleanup below to be + # skipped for precisely the accounts this lookup exists to serve. + # + # And read as a plist, not scraped with sed. `dscl`'s plain output puts a value + # containing a space on a *continuation* line ("NFSHomeDirectory:\n /Volumes/Home + # Dirs/jsmith"), so the obvious `s/^NFSHomeDirectory: //p` came back empty — for + # a home with a space in it, which is what network and relocated homes tend to + # have. + CONSOLE_HOME=$(dscl -plist /Search -read "/Users/$CONSOLE_USER" NFSHomeDirectory 2>/dev/null | + plutil -extract 'dsAttrTypeStandard:NFSHomeDirectory.0' raw -o - -- - 2>/dev/null) + # The lookup is best-effort — a directory record without NFSHomeDirectory, or any + # change in dscl/plutil output shape, yields nothing — and every consumer below + # degrades quietly: the ~/Applications half of the bundle search is skipped, and + # so is the per-user cleanup, under a closing message that says "files deleted". + # The conventional path is a better guess than no guess, and if even that is not + # there the script says so rather than reporting a clean removal. + if [ -z "$CONSOLE_HOME" ] && [ -d "/Users/$CONSOLE_USER" ]; then + CONSOLE_HOME="/Users/$CONSOLE_USER" + fi +fi + +# The bundle is looked for, not assumed. The app is allowed to register the login +# agent from anywhere under /Applications or ~/Applications (LoginItem's +# isInStableInstallLocation), so filing it into /Applications/Utilities is a +# supported thing to have done — and with APP fixed at /Applications/Dezhban.app +# that install got "the app bundle was already gone", an unloaded-for-this-boot +# agent, an `rm -rf` that deleted nothing, and a bundle that kept launching at +# every subsequent login while the script said everything was removed. +# Unconditionally, and recording *every* match rather than stopping at the first. +# Gating this on /Applications/Dezhban.app being absent left a second install +# elsewhere untouched: `SessionLock` is path-keyed and `isInStableInstallLocation` +# accepts any Applications directory precisely so two can coexist, so the copy +# holding the agent registration may not be the one at the default path. The errand +# then ran from a bundle that had never registered — where the status is `.notFound` +# and `retractAll()` truthfully reports nothing to do — `rm -rf` deleted the wrong +# copy, and the script closed with an unqualified "files deleted" while the surviving +# bundle went on starting the app at every login. +if true; then + # Root-owned, mode 700, for the same reason the errand's marker directory is: + # this runs as root and a predictable name in a world-writable directory is a + # symlink-plant away from a root write. + search_dir=$(mktemp -d "${TMPDIR:-/tmp}/dezhban-search.XXXXXX") || search_dir="" + # Every account's ~/Applications, not only the console user's. CONSOLE_HOME is + # resolved only when somebody is logged in at the console, so run over ssh or at + # the login window this searched /Applications alone — and an install in a + # ~/Applications was never found, `rm -rf "$APP"` deleted nothing, and the script + # still printed "files deleted" and, on the no-console-user branch, "Nothing will + # start Dezhban (the app is gone)". The same false clean report the unbounded + # search was added to prevent, reached from the other side. + set -- /Applications + case "$CONSOLE_HOME" in + /Users/*) ;; # already covered by the glob below + ?*) + [ -d "$CONSOLE_HOME/Applications" ] && set -- "$@" "$CONSOLE_HOME/Applications" + ;; + esac + for home_apps in /Users/*/Applications; do + [ -d "$home_apps" ] || continue + set -- "$@" "$home_apps" + done + for root in "$@"; do + [ -n "$search_dir" ] || break + [ -d "$root" ] || continue + # Any depth under an Applications directory, to match + # LoginItem.isInStableInstallLocation — a depth limit meant an install the app + # would happily register the login agent from, say + # /Applications/Utilities/Network/Tools/Dezhban.app, was one the uninstaller + # could not find, so it printed "Nothing will start Dezhban (the app is gone)" + # over a bundle still launching at every login. + # + # But pruned at every .app boundary. Unpruned, this walked *inside* every + # installed application — millions of inodes on a Mac with Xcode, repeated for + # each home directory, any of which may be a network mount that blocks — while + # the script sat silent, after `panic` had already torn the rules down and + # before anything was deleted. Nothing useful lives inside another app bundle. + # + # One file per match, named by mktemp so nothing has to be counted, and each + # path written with printf so `$(cat …)` reproduces it byte for byte. Piping + # through `head -1` truncated at the first newline — legal in a macOS directory + # name — and the result is handed to `rm -rf` as root. + # Two pruned branches, not one chain: `expr -a \( … \) -prune` evaluates the + # prune only when the whole conjunction is true, so a non-matching bundle fell + # through and was descended into anyway — verified by finding a Dezhban.app + # nested inside an Xcode.app fixture. + find "$root" \ + \( -name Dezhban.app -type d -exec sh -c ' + out=$(mktemp "$2/bundle.XXXXXX") || exit 0 + printf "%s" "$1" >"$out"' _ {} "$search_dir" \; -prune \) \ + -o \( -name '*.app' -type d -prune \) 2>/dev/null + done +fi +# The default path counts as a candidate even if the search could not run. +if [ -n "$search_dir" ]; then + for f in "$search_dir"/bundle.*; do + [ -f "$f" ] || continue + APP=$(cat "$f") + echo "note: found the app at $APP" >&2 + break + done +fi +# Every bundle found, each retracting its own registration: only the registering +# bundle can call SMAppService.unregister(), so a second copy's agent is retractable +# by nothing else. +retract_and_remove_bundle() { + APP="$1" + if [ -n "$CONSOLE_UID" ]; then + run_retraction_errand + fi + rm -rf "$APP" +} + +run_retraction_errand() { + echo "unregistering the login agent for $CONSOLE_USER ..." + if [ ! -x "$APP/Contents/MacOS/DezhbanMenu" ]; then + # Only the app can call SMAppService.unregister(), so with the bundle already + # gone — dragged to the Trash before running this — the registration cannot + # be retracted by anything here, or ever again. Saying nothing would report a + # clean uninstall over exactly the orphan this errand exists to remove. + # + # Always warned, but worded as a conditional. Gating this on + # `launchctl print` was wrong in the direction that matters: it reports only + # *loaded* jobs, and a registration sitting at `.requiresApproval` — the + # state the app documents as "the user switched Dezhban off under Login + # Items" — is registered without being loaded. So the one case this branch + # exists for printed a clean "files deleted" over a surviving entry. There is + # no root-side way to read SMAppService status, so the honest thing is to say + # "if there is an entry, remove it" rather than to claim either way. + LOGIN_ITEM_STUCK=no-app + # The marker directory is created by this script, mode 700, owned by root. The + # obvious "${TMPDIR:-/tmp}/name.$$" is a predictable path in a world-writable + # directory, and under `sudo sh` TMPDIR is often unset — so an unprivileged + # local user could pre-plant a symlink there and have the `echo` below write + # through it AS ROOT. The sticky bit stops them deleting our file; it does not + # stop them creating one first, and `rm -f` would only unlink their symlink and + # leave the window open to try again. + elif ! errand_dir=$(mktemp -d "${TMPDIR:-/tmp}/dezhban-uninstall.XXXXXX"); then + # A real skip. Announcing one and falling through anyway left errand_done as + # "/done" — on the sealed read-only system volume, so the marker could never + # be written, the poll burned its full 15s, the reason was overwritten with + # "timeout", and the kill -9 below murdered a retraction that had very likely + # just succeeded. + echo "note: could not create a private temp directory; skipping the login-item retraction" >&2 + # Its own state. Reporting this as "refused" told the user macOS would not + # retract the item and sent them to clear it by hand, when in fact nothing + # was ever attempted and simply running the uninstaller again would very + # likely retract it cleanly. + LOGIN_ITEM_STUCK=not-attempted + else + # Bounded. The errand talks to launchd over XPC, and this runs before + # `rm -rf "$APP"` — an uninstaller that hangs here, silently (output is + # discarded), leaves the machine mid-removal with no message on screen. + # + # Completion is signalled by a file rather than by watching the child: an + # exited-but-unreaped child is a zombie, which `kill -0` still reports as + # alive, so polling the pid would wait out the whole timeout on a perfectly + # successful run. And no `( sleep N; kill ) &` watchdog — that leaks its + # `sleep` past the end of the script and, if it ever fires late, aims a + # `kill -9` at a pid the system may have recycled. + errand_done="$errand_dir/done" + ( + # Written to a side name and moved into place, so the marker only ever + # appears complete. `echo failed >"$errand_done"` truncates before it + # writes, and a poll landing in that gap saw the file and `cat`ed an + # empty string — which is not "failed", so a retraction macOS had + # refused was reported as a clean uninstall. One-directional, and + # towards the silent orphan this block exists to catch. + if launchctl asuser "$CONSOLE_UID" sudo -u "$CONSOLE_USER" \ + "$APP/Contents/MacOS/DezhbanMenu" --unregister-login-item >/dev/null 2>&1; then + echo ok >"$errand_done.partial" + else + echo failed >"$errand_done.partial" + fi + mv "$errand_done.partial" "$errand_done" + ) & + errand=$! + waited=0 + while [ ! -f "$errand_done" ] && [ "$waited" -lt 150 ]; do + sleep 0.1 + waited=$((waited + 1)) + done + if [ ! -f "$errand_done" ]; then + echo "note: retracting the login item did not finish in 15s; continuing" >&2 + # Reported, not just survived. On this path no marker is ever written, so + # the "failed" test below reads false and the script would print a clean + # "files deleted" while the registration was still on file — the silent + # orphan that test exists to prevent, on the one path the timeout is for. + LOGIN_ITEM_STUCK=timeout + # The subshell AND what it started. Killing only the subshell leaves the + # DezhbanMenu it launched running, and the very next statement deletes the + # bundle out from under it — the thing the `pkill` above exists to avoid, + # reintroduced on the one path this timeout is here for. + kill -9 "$errand" >/dev/null 2>&1 || true + # Scoped to the user the errand ran as. Unscoped, this reached every + # logged-in account's menubar app on a Mac using fast user switching — + # other people's sessions, over a timeout in this one. + pkill -x -U "$CONSOLE_UID" DezhbanMenu >/dev/null 2>&1 || true + elif [ "$(cat "$errand_done" 2>/dev/null)" = "failed" ]; then + # The status matters. The app only logs a refused unregister, and this + # script discards its output — so without checking, a login item macOS + # would not retract stayed behind, pointing at a bundle deleted moments + # later and unreachable from then on, while the closing message claimed + # everything was removed. + LOGIN_ITEM_STUCK=refused + fi + rm -rf "$errand_dir" + wait "$errand" >/dev/null 2>&1 || true + fi +} + +echo "removing the app bundle(s) ..." +bundles_handled=0 +if [ -n "$search_dir" ]; then + for f in "$search_dir"/bundle.*; do + [ -f "$f" ] || continue + retract_and_remove_bundle "$(cat "$f")" + bundles_handled=$((bundles_handled + 1)) + done + rm -rf "$search_dir" +fi +if [ "$bundles_handled" -eq 0 ]; then + # Nothing found anywhere: still run the errand so the "the bundle is gone, and + # only the app could have retracted this" warning is reached. + retract_and_remove_bundle "$APP" +fi + +if [ -n "$CONSOLE_UID" ]; then + launchctl bootout "gui/$CONSOLE_UID/$LOGIN_AGENT" >/dev/null 2>&1 || true + # The app's own per-user directory: the session lock the GUI takes at startup + # to keep a second copy of itself from running, and the hand-off file beside + # it. Machine-derived, none of it the user's — and files this version creates + # that no earlier one did, so leaving them would make this script's own + # promise false. + # + # CONSOLE_HOME is resolved up top, where the bundle search needs it too. + if [ -n "$CONSOLE_HOME" ] && [ -d "$CONSOLE_HOME" ]; then + rm -rf "$CONSOLE_HOME/Library/Application Support/$APP_BUNDLE_ID" + else + # Its own flag. Sharing LOGIN_ITEM_STUCK meant that whenever an earlier step + # had already recorded timeout/refused/not-attempted, an unresolvable home + # was swallowed — the session lock and any hand-off file surviving under a + # closing report that mentioned only the other problem. + SUPPORT_DIR_KEPT=1 + fi + # The app's preferences, for the same reason. These are not cosmetic: the + # migration that moves an old LaunchServices login item onto the login agent + # records that it has run, so a surviving flag means a LATER install is never + # migrated — silently restoring the "Open minimized" bug the agent exists to + # fix. Written by the user's own cfprefsd, so it is deleted through defaults as + # that user rather than by unlinking the plist under them. + launchctl asuser "$CONSOLE_UID" sudo -u "$CONSOLE_USER" \ + defaults delete "$APP_BUNDLE_ID" >/dev/null 2>&1 || true +else + # No non-root console user: run at the login window, or over ssh on a Mac + # nobody is logged into. Every step above needs that user's own launchd + # session, so the whole per-user teardown is skipped — and because each + # LOGIN_ITEM_STUCK assignment lives inside that block, the script used to print + # an unqualified "files deleted" over an agent registration that survives, an + # entry that fails to load at every subsequent login, and a migration flag that + # makes a LATER reinstall skip the migration. The same silent-clean-report the + # other states were introduced to end. + # + # Only if nothing more specific was recorded: a console user whose uid could not + # be looked up also lands here, and "nobody is logged in" is the wrong thing to + # tell somebody sitting at the machine. + if [ "$LOGIN_ITEM_STUCK" = "none" ]; then + LOGIN_ITEM_STUCK=no-console-user + fi +fi + +# The bundles are removed by `retract_and_remove_bundle` above, each after its own +# registration has been retracted — the removal cannot be hoisted here, because only +# the registering bundle can retract, so it has to still exist when its errand runs. # The daemon's own directory: state.json, learned.json, the command file and the # control socket. All machine-derived and safe to discard — none of it is the user's. +# Every account's support directory, not only the console user's. The search above +# deliberately finds and deletes bundles in any account's ~/Applications, so scoping +# the cleanup to the console user left those users' session lock and hand-off file +# behind — under a closing message that says everything was removed. Root can reach +# these; the preferences and the login item cannot be, and are named in the warning. +for home in /Users/*; do + [ -d "$home/Library/Application Support/$APP_BUNDLE_ID" ] || continue + rm -rf "$home/Library/Application Support/$APP_BUNDLE_ID" +done + echo "removing daemon state at $STATE_DIR ..." rm -rf "$STATE_DIR" @@ -73,3 +388,51 @@ rm -rf "$SHARE_DIR" echo echo "dezhban uninstalled — rules removed, service unregistered, files deleted." +case "$LOGIN_ITEM_STUCK" in +none) ;; +*) + echo + case "$LOGIN_ITEM_STUCK" in + refused) echo "warning: macOS would not retract the login item." ;; + timeout) echo "warning: retracting the login item did not finish in time." ;; + not-attempted) + echo "warning: the login item was not retracted — the step was skipped." + echo " Running this uninstaller again will most likely clear it." + ;; + no-app) + echo "warning: the app bundle was already gone, so its login item could not" + echo " be retracted — only the app itself can do that." + ;; + no-console-uid) + echo "warning: could not look up $CONSOLE_USER's user id, so Dezhban's" + echo " per-user leftovers were not removed. This usually means the" + echo " directory service is unreachable; re-run once it is back." + ;; + no-console-user) + echo "warning: nobody is logged in, so Dezhban's per-user leftovers could not" + echo " be removed — its login item, and the saved preferences that" + echo " would make a later reinstall skip the login-item migration." + echo " Re-run this uninstaller from a logged-in graphical session." + ;; + esac + echo " Nothing will start Dezhban (the app is gone). If a \"Dezhban\"" + echo " entry remains under System Settings > General > Login Items," + echo " remove it there." + ;; +esac + +if [ "$SUPPORT_DIR_KEPT" = "1" ]; then + echo + echo "warning: $CONSOLE_USER's home directory could not be resolved, so" + echo " ~/Library/Application Support/$APP_BUNDLE_ID was left behind." + echo " It holds only Dezhban's session lock. (The saved preferences" + echo " were removed.)" +fi +echo +echo "If any OTHER account on this Mac ran the app, two things remain there that" +echo "root cannot reach: its login agent registration — nothing will start Dezhban," +echo "since the bundle is gone, but the entry lingers under System Settings >" +echo "General > Login Items until that user removes it — and Dezhban's saved" +echo "preferences, which record that the login-item migration has run, so a later" +echo "reinstall for that account would skip it. From that account:" +echo " defaults delete $APP_BUNDLE_ID"