Repository navigation
fix(app): AppServices takes the key window and app activity from the composition root, never the global NSApp - #28321
Merged
lawrencecchen merged 7 commits intoOct 31, 2026
Conversation
…n must not trap AppServices' default key-window source implicitly unwraps the global NSApp, which is nil in a test process that never made NSApplication.shared; the trap ends the whole CmuxNextAppTests process. Red on the base: the suite runs alone (swift test --filter), NSApp is nil, and the first lookup traps at AppServices.swift:87.
…n root, never the global NSApp
AppServices.keyWindowSource defaulted to { NSApp.keyWindow }. NSApp is NSApplication!, nil in a
test process that never made NSApplication.shared, so any test that built AppServices and reached a
key-window lookup (windows.active, keyWindowRole, the quit sheet, ListHandlers) trapped and ended the
whole CmuxNextAppTests process (#28275, #28090 green jobs).
- AppServices.init takes keyWindowSource with no default; AppDelegate (the composition root) passes
{ NSApp.keyWindow }, so production cannot forget it.
- The test target alone has the convenience init with no key window ({ nil }); no test call site changes.
- QuitCoordinator asks the sender (the NSApplication AppKit passed) for isActive and activate(), and
reads NSApp?.mainWindow for the sheet; the auto-archive toast reads NSApp?.mainWindow: no implicit
unwrap of the global on a path a test can reach.
Regression test: AppServicesKeyWindowSourceTests (red on the base: traps at AppServices.swift:87).
Compile job 26f2055725867054aa419409 failed: AppServices+TestKeyWindow.swift did not see LaunchReveal (CmuxNextDesign), so the convenience init did not compile and every test call site fell to the designated init (missing keyWindowSource). Every other error in that log follows from this one.
…ition root too The second trap (#28275 green eb20257db2553e2e4a4dd378, NotificationCenterService+ProgramStatusSeen.swift:57): AppServices.init starts notifications, whose viewed-program-status observation calls viewedTabs, which read NSApp.isActive. Any test that built AppServices and then awaited (each quit step awaits) trapped. - AppServices.init takes appActiveSource with no default; AppDelegate passes { NSApp.isActive }. - The notifications' six reads (viewedTabs, isViewed, the program-status visibility, arrive, the hand-off and sandbox-run banners) ask services.appActiveSource(). - The test target's init reads the application only if the test made one (NSApp?.keyWindow, NSApp?.isActive ?? false), so tests behave as before and never unwrap the global. - AppServicesKeyWindowSourceTests: the recording test also checks the app-active lookups and awaits the started observation's first ask of the injected source (no clock).
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:12 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Review (independent review subagent, arch-rise): SHIP at 3980462 PR #28321. headRefOid is equal to the reviewed head. Reviewed by reading the diff against origin/feat-cmux-next; no local build. Checks:
Notes (they do not block):
|
|
Found 10 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
…spares read the injected sources Review of #28321 (SHIP with gaps at 3980462): - FeedService read NSApp.isActive in observePresence and startIfSignedIn; its sign-in task starts with AppServices, so a signed-in test with no NSApplication trapped. FeedService takes appActive (no default); AppServices passes its appActiveSource. - Re-scan of what AppServices builds and starts: ChiefPetRoutes' appIsActive (made from the notifications service), WindowController's first .appActive focus event and NewTabSparePool's key-window reads (started after the first frame) now ask services.appActiveSource() / keyWindowSource(). - appActiveSource has its own doc; keyWindowSource's doc says debug.key replaces it alone, with the reason. - app_lines: the three joined doc comments are split again; paid by removing dead code instead: MainMenu.helpGroups, groupedItems and otherItems have no caller since #24615 (2e71847). - AppServicesKeyWindowSourceTests: the first test checks only that the lookups return (a GUI lane can have a real key window); it also calls the notifications' viewedTabs and isViewed.
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:19 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
had a problem deploying
to
ci-routing-agent
October 30, 2026 12:19 — with
GitHub Actions
Error
GitHub Actions
Error
…der, not the dead Help groups #28266 (fix/arch-rise-glass-close) already removes MainMenu.helpGroups/groupedItems/otherItems for its own lines; the second PR to land would lose its payment. MainMenu+Placements.swift is back to the base. PageFactory.homeChannelsEditRoute had no production caller (the page builds its PageEditProvider route inline); HomeChannelsComposerEditTests now builds that route the way the page does.
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:23 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:23 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 7365192 to compare; do not merge until green; see lane-rules Shipped in:
|
…e Channels edit route builder Delta review of #28321 at 923b306 (FIX): - InputObservationBuilder.keyWindow read NSApp.keyWindow; AppServices starts input verification, whose presentation-change subscription runs it (DEBUG invariant monitor, window geometry). It now takes the key window from services.keyWindowSource(). - Re-scan of subscriptions, timers and tasks AppServices starts: KeyRouter's menu gate (keyWindowFocus), the refusal HUD's window (AppActionContext, ChatsOpenCoordinator, HomeChiefActions, LinkHandlers, ViewerService) ask services.keyWindowSource(); the agent cursor snapshot, a pane's surface diagnostics (SurfaceInvariantMonitor) and SheetDismissal.endAll's default read NSApp? (never the unwrapped global). - HomeChannelsPage: PageFactory.homeChannelsEditRoute() builds the edit route and gives back its provider; homeChannelsWebPage uses it and sets run once its view exists, and HomeChannelsComposerEditTests calls it. - app_lines: paid by removing FilePageService.openEmpty, which has no caller.
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
ci-routing-agent
October 30, 2026 12:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Default changesThis pull request changes 2 defaults at
Needs a review. It passes with an independent review receipt on this commit: a comment or review whose first line is Merge receipt: merged as Regenerate the snapshot with |
Contributor
Author
|
Review (independent review subagent, arch-rise): SHIP at c45de7b Scope: delta review of
Notes that do not block:
|
lawrencecchen
added
the
ready-to-land
lawrencecchen
added a commit
that referenced
this pull request
Nov 1, 2026
…n thread on them (#28275) * test(quit): a second quit signal arms its deadline before it waits on the agent panes' state files The forced quit (ForcedQuitExit, a second SIGTERM/SIGINT/SIGHUP while a quit runs) flushed the agent panes' state files in `prepare`, under allowBlocking(.terminate), before its 160 ms deadline was armed: with a save queued behind a busy disk the exit waited up to 2 s on the main thread, and its own deadline did not bound that wait. This commit extracts the exit's steps into AppDelegate.secondSignalExit (same steps, injectable deadline and end) and adds QuitStateFilesTests.theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles. It is red on this commit: the deadline is armed about 2 s after the start, and the exit ends before the queued draft is on disk. The fix is the next commit. Not run here (no Swift build on this VM): the fleet runs it. * fix(quit): state files have one owner each and no quit blocks the main thread on them Rise audit (arch-ratchet tip rises with no reviewed exception) of cmux#27749 and #25535: - AgentPaneStateFile's process registry (`static let files = Mutex<...>`) becomes the AgentPaneStateFiles instance that AppServices owns and passes to AgentUnsentDrafts and AgentPaneScrollAnchors (composition root, PATTERNS.md). Same one-writer-per-path rule. - MobileHostPairedDevicesFile's `static let queue` and `static let inForce` become members of one file object that IPhoneHostService owns per state directory (it built a new value per use before). This run's record of the list in force is now per file object; a relaunch is a new object. - The three allowBlocking(.terminate) flushes are gone: - ForcedQuitExit `prepare` (AppDelegate): the flush ran before the 160 ms deadline was armed, so a second quit signal could wait 2 s. The exit now arms the deadline first and awaits the state files beside the Home cache's write in `save`. - applicationWillTerminate (AppDelegate) and IPhoneHostService.stop(): up to 2 s each, 4 s in all. The quit's `.terminateLater` phase (QuitCoordinator) now awaits AppServices.stateFilesDrained() in prepareWindows and again in stopBrowserEngines, next to home.flushCacheForQuit(): one 2 s limit for all the files, holding no thread. - QueueDrain (CmuxNextWakeups): waits for serial queues with one continuation and one DemandTimer deadline, so no thread blocks and a quit with no backlog costs no timer fire. - SignalRelay moves from CmuxNextApp/Quit to CmuxNextControl/Diagnostics (beside ForcedQuitExit and QuitDeadline), so CmuxNextApp's lines do not rise. Ratchet (arch_counts --delta): global_mutable_statics -3, blocking_allow_escapes -3, app_lines -31. Regression: QuitStateFilesTests.theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles is red on the previous commit and should be green here; theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread covers the normal quit's wait. Not run here (no Swift build on this VM): the fleet runs them. Owners told: Performance Lead (Wakeup primitives, statics), PL-threads (the terminate allowance). * refactor(quit): the quit's steps and its engine stop are QuitCoordinator members `steps()` builds the QuitSteps that `complete` runs, and `stopEngines` is the engine stop (the deadline, then CefShutdown) that stopBrowserEngines ends with. Same steps in the same order; a test can now run a step through the coordinator with its own engine stop. No behavior change. * test(quit): a quit through QuitCoordinator waits for a queued draft before it goes on QuitStepsStateFilesTests runs the coordinator's own steps with a new chat's draft save queued behind a busy disk (a held file queue, released after 1 s): - theSaveStepEndsWithTheQueuedDraftOnDisk: prepareWindows returns with the draft on disk. - theEnginesStopAfterTheQueuedDraftIsOnDisk: stopBrowserEngines reaches the engine stop with the draft on disk. Each fails when its step's `await services.stateFilesDrained()` is removed (the red-proof ref). Not run here (no Swift build on this VM): the fleet runs them. * refactor(wakeups): QueueDrain races its queues and its limit through FirstOutcome One path for "first of an event and a DemandTimer limit wins" (FirstOutcome): QueueDrain keeps only the count of queues still to answer; its own continuation and countdown class are gone. * test seam(quit): AgentPaneStateFiles tells when a drain begins `AgentPaneStateFiles(drainBegan:)` is told each time `drained(within:)` begins, before it waits; AppServices takes the registry through its init (default: a plain one). A test holds a file's queue and releases it only there, so only a real drain lets a quit step go on: no wall-clock hold decides a test (lead rule R5). `stateFilesDrained` returns whether every file's saves ran in time. * test(quit): only a drain of the state files lets a held save go; no clock decides the quit tests The quit tests held a file's queue and let it go on a timer (1 s), or measured wall time, so a slow test host could decide them (lead rule R5). DrainGate holds the queue until the registry is told a drain began (`AgentPaneStateFiles(drainBegan:)`), and records the order of the quit's steps: - QuitStepsStateFilesTests (both): a step that never awaits the state files never writes the draft, and the gate saw no drain. - theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles: the order is "deadline armed", then "drain began", and the exit ends with the draft on disk. - theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread: the main actor lets the held save go once the drain began; a wait that blocked it would reach its limit with the draft still queued. * test(quit): the quit tests' services have no key window (no app object in the test process) The green step 04224ccd at 07f1b95 crashed both quit suites before any assertion: AppServices.keyWindowSource reads `NSApp.keyWindow`, and `NSApp` is nil in a test process that never made NSApplication.shared; the quit steps ask for the key window (WindowManager.swift:143). The tests' services answer nil instead, with no app object. * test(quit): DrainGate and its log are nonisolated (the drain calls them off the main actor) The compile a1714da9 at d77dbff refused QuitStateFilesTests.swift:85: the drainBegan closure runs in the drain, off the main actor, and called DrainGateLog.append, main-actor by the test target's default isolation. Both helper classes are nonisolated now (a Mutex and semaphores inside). * test(quit): the quit tests make the app object first, and the non-blocking wait has a time limit The green step eb20257d at 57dfaa1 crashed both quit suites before any assertion: a quit step reads `NSApp.isActive` (NotificationCenterService+ProgramStatusSeen.swift:57), and `NSApp` is nil in a test process with no app object. The one helper makes NSApplication.shared first, as the other AppServices tests do; the root fix of those reads is fix/arch-rise-appservices-no-nsapp-trap. theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread gets a 1 minute time limit, so a wait that never ends fails the test instead of the run. * fix(quit): no bare drain limit; the last drain follows the engines' close; a lost draft is named and told R9 review of #28275 (reviewer 0053c0be): - R-L1 (bare 2 s): the state files' drain has no limit of its own. The quit awaits it only after its deadline is armed: QuitCoordinator's engine stop now arms QuitDeadline.engineStopLimit (15 s), drains, closes the engines (`closeEngines`, a seam for tests) and drains again, so a page that closes and saves its draft (the composer's page-hide flush) is on disk when the quit ends. The forced quit awaits it under its 160 ms deadline. The prepareWindows drain is gone (the engine stop covers it). QueueDrain is deleted: each file awaits its own queue (`AgentPaneStateFile.saved()`). - R-L5 (silent loss): when a quit deadline ends the process while a state file still saves, its end (`AgentPaneStateLoss.deadlineEnd`) logs a fault naming the files (no content) and records them in `<state>/agent-state-lost.json`; the next launch takes the record and, for a new chat's unsent text, shows a toast ("Text typed in a new chat just before cmux quit was not saved.", 21 locales). Both deadline reasons name the state files. - Tests (no clock decides any): QuitStepsStateFilesTests (the engines close after the queued draft is on disk; a draft saved while the engines close is on disk when the quit ends), AgentPaneStateLossTests (the deadline records the files still saving, the next launch takes them once, the notice is only for a lost draft), AgentPaneStateFileTests (a file is saving until its saves ran). Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): a paired-phones save a quit deadline cuts off fails closed, is named and told R9 follow-up on #28275: with the drain's 2 s limit gone, a paired-phones save the quit deadline cut off was silent, and an unsaved removal kept the old list's seal, so a phone the person removed counted again after a relaunch. - MobileHostPairedDevicesFile counts its queued and running saves and withdrawals (`isSaving`), and `failClosedIfSaving()`, called on the deadline's thread, stops every list counting in this run and clears the seal when one is still saving: after a relaunch the stored list no longer matches its seal, so every phone pairs again (the digest rule of `load`). A save that still finishes before the process ends records its own new list. - MobileHostPairedQuitWatch, owned by IPhoneHostService and set whenever its file changes, gives the deadline's end that file from any thread. AppServices.quitDeadlineEnd() builds the one end both deadlines use: it fails the paired list closed, logs a fault naming every file still saving (names only), and records them in agent-state-lost.json. - The next launch logs a cut-off paired list and shows a second toast: "A change to your paired iPhones was cut off when cmux quit. Pair your iPhones again." (21 locales). - Tests (no clock decides): MobileHostPairingTests.aRemovalCutOffByAQuitDeadlineIsNotTrustedAfterARelaunch, AgentPaneStateLossTests.aCutOffPairedListIsRecordedAndTold, QuitStateFilesTests.theLossRecordNamesThePairedListByItsFileName. Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): the loss notice uses the shared toast center inside (its type is not public API here) The compile 6c6043c8 at c85dcca refused AgentPaneStateLoss.showNotice: its `toasts:` parameter default named CmuxToastCenter, an internal import of CmuxNextAgentPane, in a public method. The paired-phones test waits for the held clear in a synchronous helper, not in its async body. * fix(quit): the deadline's end never calls the Keychain; the next launch clears a cut-off paired seal R9 review of #28275 at 96a1de8 (R-L5): the deadline's end cleared the paired list's seal (SecItemDelete) on the deadline thread before `_exit`. A hung securityd, the case QuitDeadline exists for, would hold the process forever, and the fault and the record came after it. - The deadline's end only marks a paired-phones save still in flight (`paired-devices.cut-off` beside the file, `MobileHostPairedDevicesFile.markCutOffIfSaving()`), then logs, records and ends. - The next launch's file object clears the seal first on its own queue, before any read (`distrustIfCutOff()`): no stored list counts in that run, and after it the list no longer matches its seal, so every phone pairs again. A Keychain that refuses the clear keeps the list refused for the run and the marker for the next launch (a fault). - The pair-again toast shows only when this launch cleared the seal (`sealClearedForCutOff()`). - The second drain's comment says it catches the page-hide flush of Chromium-hosted agent pages. - Test: MobileHostPairingTests.aRemovalCutOffByAQuitDeadlineIsNotTrustedAfterARelaunch now checks the deadline's end leaves the seal untouched while a removal is held before its own clear, and that the next launch clears the seal before its first read and trusts no phone. No clock decides it. Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): a cut-off paired save fails closed even when its marker is not written; notices never wait on each other R9 review of #28275 at 67afa2c (two silent-loss notes, R-L5): - The deadline's end ignored a failed marker write and still named the paired file, so on a full disk a removed phone's old list counted again after a relaunch with no notice. Now a failed write is a fault, and the record names the file with the note "paired-devices.cut-off (not written)" (`MobileHostPairedDevicesFile.CutOff.unmarked`). At the next launch the paired file fails closed when its marker exists OR the last quit's record names it (`lastQuitCutOff`, asked on its queue before any read); a refused seal clear then writes the marker for the launch after it. The record is read once per launch (`AgentPaneStateLoss.lastQuitNames()`), so the paired check and the notice share it in either order. - The launch awaited the paired seal check (a Keychain call) before it showed the unsent-draft notice. `showNotices` shows the draft notice at once and the pair-again notice when its own check answers. - Tests (no clock decides): MobileHostPairingTests.aCutOffRemovalWithoutItsMarkerIsNotTrustedAfterARelaunch (an injected disk refuses the marker), the deadline-end test counts every seal call (zero during the end), AgentPaneStateLossTests.thePairedCheckAndTheNoticeShareOneReadOfTheRecord. Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): no public default names an internally imported symbol; the launch notice takes a closure The compile c822c59f at fc54bef refused MobileHostPairedDevices.swift:198: the public init's `writeMarker` default called `blockingWrite` (an internal import of CmuxNextCompat). The parameter is optional now and the init picks the default inside its body. AppDelegate passes the paired seal check to `showNotices` as a closure literal, not as a partially applied main-actor method. * fix(quit): a launch told only by the record keeps the cut-off fact on disk until its seal clear ends R9 review of #28275 at e22c234 (R-L5): the record is read once and removed, so on the record-only path (the marker write failed at quit) a launch whose seal clear hung, then a quit or crash, left the launch after it with no record, no marker and the old seal: the removed phone was trusted silently. - When only the record names the cut-off save, `distrustIfCutOff` writes the marker BEFORE the clear; a failed write is a fault of its own. - The clear counts as a save in flight (`saving`), so a quit deadline during a hung clear marks and names the file again. - A refused clear keeps the marker for the next launch (its fault says so). - Test (no clock decides): MobileHostPairingTests.aRecordOnlyLaunchWhoseClearHangsStillLeavesTheListUntrusted: a record-only launch whose clear is held; a deadline during it finds `.marked`; a third file object with no record trusts no phone. aCutOffRemovalWithoutItsMarker... drains its first file object's launch step before the marker can exist. Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): the record keeps naming a cut-off paired list until the fact is safe; no clock in the removal tests R9 review of #28275 at d6f29ce (R-L5): the launch removed the record at its first read, so when the launch's own marker write failed too, the fact lived only in that process: a refused clear (then a normal quit) or a hung clear (then a crash) left the next launch with no record, no marker and the old seal, and the removed phone was trusted silently. - AgentPaneStateLoss keeps naming the paired list in the record after the launch reads it (only that name: a lost draft is told once), a quit deadline in that run writes the name beside its own, and `pairedCutOffSettled()` removes it. - MobileHostPairedDevicesFile tells `cutOffSettled` once the fact is safe without the record: its marker is on disk (written at launch before the clear) or the seal is cleared. IPhoneHostService takes the app's record (`pairedQuitRecord`) for both questions. - The refused-clear fault says what keeps the fact: the marker, or the record when no marker exists. - R5: aRemovalTakesTheOldDigestAwayAfterTheSavesBeforeIt and aRemovalNeverWaitsOnTheCallersThread release their held save on a signal (after the removal is queued), not after 0.3 s / 0.6 s; the second no longer measures time: a removal that waited on the caller's thread would never return, and its time limit fails it. - Tests (no clock decides): MobileHostPairingTests.aRecordOnlyLaunchWithNoMarkerAndARefusedClearKeepsTheRecord, AgentPaneStateLossTests.theRecordKeepsNamingACutOffPairedListUntilItIsSettled. Not run here (no Swift build on this VM): the fleet runs them. * fix(quit): quitSteps takes the quit's choice (endLocalSessions ends with it); PhoneAdmissionStoreTests need no forgetThisRun Fleet compile 6ade1fafcc18 at b062435: QuitCoordinator.swift:215 had no 'choice' in quitSteps(), and two feat tests called the static per-path forgetThisRun, which the per-file inForce record replaces: the restored store's new list file remembers no digest, as a relaunch does. * test(quit): the test AppServices initializer takes the state files (feat's #28321 test init replaced the one QuitStateFilesTests called) Suites step 4e05c54c690b at 729a0b7: QuitStateFilesTests.swift:33 passed agentPaneStateFiles to feat's test-only convenience init, which had no such argument. * test(drafts): feat's UnsentDraftTests share one AgentPaneStateFiles per test (the per-app writer registry) Compile 7b999930c9b2 at 0ff4259: UnsentDraftTests.swift built AgentUnsentDrafts(file:) with no files. Each test now gives its instances one AgentPaneStateFiles, so a fresh instance reads after the queued save as the static registry made it do. A grep of the package finds no other caller of a changed initializer without its new argument. * fix(quit): the state-file drain before the engines close is held to the time they do not need Review of e79b754 (comment 6554069134): an unbounded drain before closeEngines could use up feat's engineCloseWait (a hung Keychain call on the paired-phones queue, a loaded disk), so CefShutdown lost its flush. The first drain is now a QuitStepLimiter step limited to the deadline's time left less the reply margin and engineCloseWait; past it, it runs beside the close and the quit awaits it in finishOverruns. The drain after the close is unchanged.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AppServices read the global
NSAppin two places that a test reaches with no application object.NSAppisNSApplication!. In a test process that never madeNSApplication.sharedit is nil, and the implicit unwrap traps. The trap ends the whole CmuxNextAppTests process and hides every later test. It broke #28275's green steps twice (04224ccd89106f08f50cc908 and eb20257db2553e2e4a4dd378), and #28090's green step.Sites fixed, with the commit that added each:
Sources/CmuxNextApp/AppServices.swift:87var keyWindowSource ... = { NSApp.keyWindow }(added in 7660c2b). Reached bywindows.active,keyWindowRole, the quit sheet,ListHandlersandeventDidBringForward.Sources/CmuxNextApp/Notifications/NotificationCenterService+ProgramStatusSeen.swift:57guard let services, NSApp.isActive(added in e5df76a).AppServices.initstarts the notifications service, and its viewed-program-status observation callsviewedTabs. So any test that builds AppServices and then awaits traps. This is what fix(quit): state files have one owner each and no quit blocks the main thread on them #28275's quit tests did.Sources/CmuxNextApp/Feed/FeedService.swift:87, :127, :135NSApp.isActive(the owner's presence). AppServices builds FeedService, and its sign-in task reachesstartIfSignedIn, so a signed-in test with no application object traps there too.The fix (composition root, PATTERNS "composition root"):
AppServices.inittakes two required parameters with no default:keyWindowSource: @MainActor () -> NSWindow?andappActiveSource: @MainActor () -> Bool.AppDelegatepasses the real ones ({ NSApp.keyWindow },{ NSApp.isActive }). It is the only production call, and it does not compile without them. AppServices itself reads noNSApp.FeedService.inittakesappActivewith no default. AppServices passes itsappActiveSource.ChiefPetRoutesappIsActive: the notifications service makes the pet.WindowController's first.appActivefocus event.NewTabSparePool's two key-window reads: the pool starts after the first frame.InputObservationBuilder.keyWindow:input.startsubscribes tocache.presentationChanges, and the DEBUGInputInvariantMonitortimer and the window-geometry observer runobserve. It now takes the key window fromservices.keyWindowSource().KeyRouter+HookskeyWindowFocus(the menu key-equivalent gate that AppServices installs on the registry).AppActions.bind(services)installs (AppDelegate calls it, and so do 102 test files), reached when an action is refused in a process with no window controller, now askservices.keyWindowSource()too: the refusal HUD's window inAppActionContext+Targets(the registry's refusal observers),ChatsOpenCoordinator,HomeChiefActions,LinkHandlers,ViewerService. AppServices construction does not reach these; a test that binds the actions does.NSApp?and never unwrap the global:AgentCursorSnapshotBuilder.orderedControllers(the agent cursor visibility source),PaneController.visiblePageWindowsOverContent(SurfaceInvariantMonitor's check, subscribed inAppServices.init) andSheetDismissal.endAll's default (QuitCoordinator.shouldTerminate).services.appActiveSource():viewedTabs(ProgramStatusSeen:57),isViewed(Lookup:42), the program-status visibility (ProgramStatus:67),arrive(NotificationCenterService:454), the hand-off banner (HandoffNotices:151) and the sandbox-run banner (SandboxRunNotices:83).init(environment:launchReveal:agentPaneEnvironment:)(Tests/CmuxNextAppTests/AppServices+TestKeyWindow.swift). It passes{ NSApp?.keyWindow }and{ NSApp?.isActive ?? false }. These are optional reads: a test that madeNSApplication.shared(197 files do) or runs in the GUI lane, where a window can really be key and the app active, behaves as before. A test with no application object gets no key window and an inactive app. No call site can trap, and no test call site changed.NSAppuses changed (same lines, no behavior change):Quit/QuitCoordinator.swift:105and:135:NSApp.isActiveandNSApp.activate()becomesender.isActiveandsender.activate().senderis the NSApplication that AppKit passes toshouldTerminate.Quit/QuitCoordinator.swift:245(the quit sheet's window) andAppServices+AutoArchiveHost.swift:13(the auto-archive toast):NSApp.mainWindowbecomesNSApp?.mainWindow.NSAppuses left. I scanned everyNSApp.use in CmuxNextApp and traced what AppServices builds, starts and subscribes. None of these is reached from AppServices construction, the tasks, subscriptions and timers it starts, or the quit step bodies. Each runs only from a person's action, an AppKit event or delegate call, a debug socket command, or code thatAppDelegateinstalls:AppDelegate.swift(the real sources),MainMenu.swift,HangReports/WatchdogReporting.swift,Control/AppControl.swift:184(a debug call on the control socket).Control/Debug*.swift,Focus/DebugFocus.swift,Focus/DebugKey*.swift,InputVerification/DebugMouse.swift,InputVerification/DebugNativeInput.swift,Settings/DebugSettings.swift,HostPressure/HostPressureIndicatorView+Debug.swift,RemoteView/RemoteBrowserPages.swift:257,Windows/WindowInvariants.swift,DebugNowPlaying.swift,Control/CaptureSceneRegistry+HomeAgents.swift,Control/UpdateHarnessProbe.swift(an update-harness run only).Focus/key and event handling (KeyRouter+NewTabInput,CreationInputCoordinator,FocusEffectApplier,WindowScopedShortcutHintModifierMonitor,Keybindings/KeybindingsPageProvider),FocusCoordinator.swift:266(NSApp.currentEventonly after(NSApp as? CmuxApplication)?.isHandlingSendEvent() == true),BrowserHost/AppBrowserHost.swift:184,TerminalHostDelegate.swift:123,IconPicker/IconPickerService+BookmarkAnchors.swift,Windows/WindowController.swift:351(windowDidResignKey), drag sessions (Drag/,Screens/ScreenDragSession.swift).Handlers/(WindowHandlers,WindowTargeting,OnboardingHandlers,KeymapHandlers,CLIInstallHandlers,IconHistory,NowPlayingCardHandlers,TabIconHandlers,BrowserDownloadsPopup), import and onboarding sheets (Onboarding/,Bookmarks/BookmarkBrowserImport.swift,ChiefMemoryImport/),Cloud/CloudMove.swift:340,Density/DensityController.swift,Hints/WindowHintController.swift,Pins/PinCommands.swift,Remote/AddDeviceServices+App.swift,Server/ServerMenuBarController.swift,SessionTransfer/,SleepyMode/,WebInspection/WebInspectorTargets.swift,Windows/WindowController.swift:405(a close),Control/AppBrowserPasskeyHost.swift(a page's passkey request),Notifications/HandoffNotices.swift:208(a banner click).Focus/InputVerificationService+NoActivateGuard.swift,Focus/NoActivateKeyboardGuard.swift,Windows/WindowManager+FullScreen.swift:23(behindrelaunchBehind == nil ||).AppServices.swift:615-693(NSApp as? CmuxApplication)?;:635readsNSApp.keyWindowinside a closure that is set only whenNSAppis aCmuxApplication.Not in this PR: #28275 adds
QuitCoordinator.steps()and the replaceablestopEngines. This PR does not copy them. #28275's own quit tests cover the quit steps end to end.Ratchet delta
python3 scripts/cmux-next/arch_counts.py --delta origin/feat-cmux-next HEAD:app_lines -3 {'CmuxNextApp': -3}, every other row +0. The new lines are paid for by removingFilePageService.openEmpty(in:focus:)(Pages/Files/FilePageService.swift), which has no caller. No openfix/arch-rise-*branch changes that file.Also changed:
PageFactory.homeChannelsEditRoute(run:)(added in 2ede214) had no production caller, because the Channels page built its edit route inline, soHomeChannelsComposerEditTeststested a copy. NowhomeChannelsEditRoute()builds the route and gives back itsPageEditProvider.homeChannelsWebPageuses it and setsrunonce its view exists, and the test calls the same builder.arch-ratchet.py --base origin/feat-cmux-next --head HEAD: no rise.Testing
Regression suite
AppServicesKeyWindowSourceTests(Tests/CmuxNextAppTests/AppServicesKeyWindowSourceTests.swift):keyWindowLookupsWithNoApplicationObjectDoNotTrap: builds AppServices with the test init and callskeyWindowSource(),windows.active,keyWindowRoleand the notifications'viewedTabsandisViewed. It checks only that they return: on a GUI lane an earlier test can leave a real key window. It is the only test that is red-first in history (ef26e0f, before the fix; that version also expected nil values). On the base it traps atAppServices.swift:87. The fleet runs a suite alone in its own process (swift test --filter), where no application object exists.everyLookupAndTheStartedTasksAskTheInjectedSources: this test came with the fix, because it uses the new init parameters and cannot compile on the base. It passes recording sources and checks:windows.activeandkeyWindowRoleeach ask the key-window source once.viewedTabsandisViewedask the app-active source (else it fails at once and does not wait).FirstOutcome, no clock).This test also fails on red when an earlier test already set
NSApp, where the trap cannot show.Red-proof ref (NEVER MERGE):
fix/arch-rise-appservices-no-nsapp-trap-redproof-4at d0bb5a7. It is the head minus the source fix. AppServices and FeedService keep their new parameters, because the test target needs them to compile, but readNSApp.keyWindowandNSApp.isActiveagain. Every other file whoseNSAppread the fix changed is back to the base. The Channels edit route builder and theopenEmptyremoval stay, because they are not part of the fix. The test files are byte-identical to the head.Fleet:
LaunchReveal(CmuxNextDesign). Every other error in that log followed from it. Fixed in 6f7244f.CmuxNextApp/AppServices.swift:497: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value; the suite process ended with signal 5.HomeChannelsComposerEditTests.Note: 197 of the 199 test files that build AppServices still call
_ = NSApplication.shared. Two of them say that they do it for this read:ChiefPendingRemoveTests.swift:31("the services read NSApp") andNewTabDropdownPaletteActionsTests.swift:13("the palette's context reads NSApp's key window"). Most of the others also make windows or AppKit views, so this PR leaves all of them. The two files without it areBackgroundAccessoryPolicyTests.swiftandHostPressureIndicatorTests.swift. They useNSApplication.sharedin other ways.Observed / Not run
Observed: the prepush gate (no new violations),
arch-ratchet(no rise),arch_counts --delta(app_lines -3, others +0), the namespace lint (OK), the fleet compile-checks above, the failed compile's error log and the green step at 3980462 (2 of 2 tests passed).Not run: No test signs in to drive FeedService's presence: the feed reads were checked by reading only. I did not run the full CmuxNextAppTests on the fleet. I did not build or test anything on the local machine (repo rule). I checked by reading, not by a test, that the
NSAppuses left are not reachable from construction or the quit steps.Fleet red / green (same focused suite, final head c45de7b)
fix/arch-rise-appservices-no-nsapp-trap-redproof-4, never merged: AppServices and FeedService readNSAppagain, the other fixed reads back at base; test files byte-identical), suiteAppServicesKeyWindowSourceTests, job a3809497da2962e79bc694a3:Build complete! (186.04s), thenkeyWindowLookupsWithNoApplicationObjectDoNotTrap()started and the process trapped:CmuxNextApp/AppServices.swift:502: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value(the red ref's{ NSApp.keyWindow }), the suite process ended with signal 5. Run without--red(the client refuses it when the red commit adds no test file).Build complete! (446.20s);AppServicesKeyWindowSourceTests:Test run with 2 tests in 1 suite passed;HomeChannelsComposerEditTests:Test run with 1 test in 1 suite passed.Changelog
none