Repository navigation
fix(windows): a window closed during launch stays closed when the restore's workspace is reported late - #31155
Conversation
…restore ended, opens a window the person closed BackgroundAccessoryPolicyTests.aWindowClosedBeforeTheRestoreFinishesStaysClosed failed 1 run in 3 on cmux12s (job 691124a2e9b4323980de7e07): the restore created its first workspace, the create's reply came before the store applied the event that lists it, and the restore ended first. It cleared closedDuringRestore and closedLaunchWindowID (WindowManager.swift:246-249), so the late report's reconcile (WindowManager+Membership.swift:497) gave the workspace a new fallback window and presented it: the window the person closed during the launch came back. The new test makes that order certain instead of a race: a test seam on the store (DaemonStore.eventHold, nil in the app) keeps the batch with the new workspace buffered until the restore ended, then releaseHeldEvents() applies it. It expects no window presented, the workspace in a closed window, and the memory of the closed launch window gone afterwards. Expected red at this commit.
… workspaces the daemon reports after the restore ended The restore forgot at its end that the person closed the launch window (closedDuringRestore, closedLaunchWindowID). Its first workspace is created during the restore, but the create's reply can come before the store applies the event that lists it; the late report's reconcile then found no window and no memory, registered a new fallback window and presented it. That memory is now one value in CmuxNextLayout (RestoreClosedWindows), next to WindowRegistry. The restore records the workspace it asked for that the store does not list yet, with the event sequence taken after the create's reply. If the person closed the launch window, the memory of that window outlasts the restore: reconcile still gives a late workspace the closed launch window as its fallback and keeps it closed in the same registry step, and ends the memory once each such workspace is in a window, or the store applied past the barrier without listing it (gone), so a memory with nothing left to wait for never takes an unrelated workspace. The check runs in reconcile, is O(the restore's workspaces), and adds no main-thread wait. RestoreClosedWindowsTests covers the gone and reopened cases the app test does not reach.
|
Review (independent review subagent, Opus 5.5 claude-opus-5-5, read-only): SHIP at ae9ad32 The reviewer confirmed:
Low follow-ups, not blocking:
Owners: no OWNERS row covers WindowManager, CmuxNextLayout/Windows or the Swift DaemonStore. Lawrence (DaemonStore+Driver), Leo (recent WindowManager work) and PL-switch (WindowRegistry+Selection, WindowState, WindowController) should be told. |
|
Found 7 test failures on Blacksmith runners: Failures
|
|
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); base red: the same failure on the base ba68875; does not block Shipped in:
|
* cmux-next tests: batch B1 suites close the windows they open Leo (2026-10-28), on Swift suites that leave windows open for the suites after them: "But why do you guys not just clean up after yourselves, or at least do your best to?" Each suite in this batch now closes every window its tests open, in the test's own teardown, and carries `.closesItsWindows` so a new leak fails by name: - teardowns that only ordered a window out close it; - tests that make browser pages release them (or close the chrome's popup panels), and tests that open the palette close its panel; - background mode's menu bar item goes when the launch test shuts down. Product code never closed three kinds of panel, only ordered them out, so they stayed in the app's window list while anything held their owner: - a window's overlay host panel, when the window closed (WindowOverlayHost); - a browser page's Page Info and Extensions panels, when its tab closed (BrowserEntry.close now calls BrowserChromeView.closePopupPanels); - the Mac screen approval prompt's panel, whose content holds the prompt (now closed and dropped when the prompt hides; the next request makes one). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * cmux-next: a closed window makes no new overlay panel; popups close their panels Leo (2026-10-28): "But why do you guys not just clean up after yourselves, or at least do your best to?" - WindowOverlayHost keeps a closed window's torn-down host registered, so a late `host(for:)` for that window returns it instead of making a fresh panel nothing closes; a window shown again gets a new host, and `existingHost` still answers nil once torn down. - NativePopupHost.closePanel closes a popup's panel for good; Page Info, the Extensions bubble and the host pressure popover (when its indicator leaves its window) use it. - DebugPaletteCaptureTests and HomeOpenFocusesComposerTests drop the trait for now: the first leaves only AppKit's own TUINSWindow, the second the Chief Settings popover's panel, which its owner never closes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * cmux-next tests: Home's Chief Settings panel closes with its test; AppKit-only suites keep the trait Leo: "But why do you guys not just clean up after yourselves, or at least do your best to?" Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * Tests: batch B2 suites close the windows they open Leo (2026-10-28): "But why do you guys not just clean up after yourselves, or at least do your best to?" - WindowOverlayHost.tearDown closes its overlay panel when its window closes (it only ordered it out, so every closed window left an open OverlayHostPanel in the app's window list). - Test windows close instead of ordering out (isReleasedWhenClosed = false first); harnesses that never closed their window now do. - Hover card, tooltip and palette windows the code under test opened close in teardown. - App harness tests (withApp, the multi-view fixture) close the windows their app opened, including the page chrome's Page Info and Extensions bubbles. - Each fixed suite carries .closesItsWindows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * WindowOverlayHost: a closing window's overlays stay on its torn-down host; app harness releases its pages Leo (2026-10-28): "But why do you guys not just clean up after yourselves, or at least do your best to?" A toast or overlay presented while a window closes (a toast's dismiss that offers the next one) reached a new host for the closed window and opened a new panel nothing closed. The torn-down host answers for the window until the close is over and never attaches its panel again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * WindowOverlayHost: a host made during its window's close tears down with the close Leo (2026-10-28): "But why do you guys not just clean up after yourselves, or at least do your best to?" A window controller's teardown (run from the close) could make the window's first overlay host; its close observer came after the close notification, so its panel stayed open. The window manager tears that host down after the controller's teardown. The palette test ends the search field's editing before it closes the palette. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * MenuKeyRoutingTests, MultiWindowInputRoutingTests: harness close closes what the app opened for its windows Leo (2026-10-28): "But why do you guys not just clean up after yourselves, or at least do your best to?" Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * Drop the WindowManager hook: WindowController.teardown already tears down a host made during the close Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-85ca59f74f02d81734c3 * Register fullscreen test window cleanup before throwing setup Agent-Session: https://cmux.com/deeplink/agent?id=aid-8adba6ac6b1b136fb9ca * Close browser profile test pages and palette on early failure Agent-Session: https://cmux.com/deeplink/agent?id=aid-c5f7357864b9736bff29 * Close routing harness windows when setup throws Agent-Session: https://cmux.com/deeplink/agent?id=aid-8adba6ac6b1b136fb9ca * Finish B2 fixture cleanup on early setup failures Agent-Session: https://cmux.com/deeplink/agent?id=aid-8adba6ac6b1b136fb9ca * Make window test cleanup cover failed setup and all cached pages Agent-Session: https://cmux.com/deeplink/agent?id=aid-c5f7357864b9736bff29 * fix(tests): drain owned daemon startup and settle approval panels Agent-Session: https://cmux.com/deeplink/agent?id=aid-c5f7357864b9736bff29 * Reap both Stash cross-machine harness owners after failed startup Agent-Session: https://cmux.com/deeplink/agent?id=aid-8adba6ac6b1b136fb9ca * Propagate cross-machine startup cancellation into owned teardown Agent-Session: https://cmux.com/deeplink/agent?id=aid-8adba6ac6b1b136fb9ca * fix(lifecycle): drain discarded startups and retire page info windows Agent-Session: https://cmux.com/deeplink/agent?id=aid-c5f7357864b9736bff29 * fix(tests): close palette panels after visibility ends Agent-Session: https://cmux.com/deeplink/agent?id=aid-c5f7357864b9736bff29 * fix(tests): avoid throwing key path in browser retirement assertion Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): avoid throwing key path in browser retirement assertion Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): preserve launcher endpoint reuse during teardown Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): preserve launcher endpoint reuse during teardown Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): synchronize moved tab metadata before assertions Wait for the destination daemon store through its existing sync barrier before inspecting moved tabs. Keep metadata assertions unchanged so a failed rename still fails the test. Validation: prepush gate and diff check pass. Native Swift syntax is unavailable in this Linux lane; CrossMachine/Stash fleet rerun required. Prior fleet runs 0e700eba and da960326 failed the name assertion before this barrier. Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): finish fresh launch teardown on cancellation Propagate cancellation from the presentation wait, capture its error, and join owned daemon shutdown before rethrowing. Register window and directory cleanup before startup so their lifetime ends in order. Validation: static prepush and diff checks pass. Swift syntax/native execution unavailable in the Linux lane; fleet regression and cumulative review requested. Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): retain owned daemon cleanup on handshake failure Record verified process identities and use the existing audit-token fallback when a shutdown handshake fails. Preserve state directories until every owned daemon exit is confirmed, and bound moved-tab synchronization and cancellation. Validation: cmux-next prepush gate and diff check pass; native Swift compilation/runtime unavailable in this Linux lane. Fleet reruns and exact-head review required. Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): avoid region checker error in barrier wait Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): capture barrier daemon before task group Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): observe daemon exit by kernel identity Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * test: preserve unverified daemon ownership after shutdown Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): retain cancelled daemon ensures through teardown Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): remove duplicate palette panel cleanup Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(tests): keep waiting when daemon identity is unreadable Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(windows): skip presentation after close during restore Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * fix(windows): record close intent before restore races Agent-Session: https://cmux.com/deeplink/agent?id=aid-66d4d9063f5f1bfabf11 * Revert "fix(windows): record close intent before restore races" This reverts commit 141a41b. Agent-Session: https://cmux.com/deeplink/agent?id=aid-1a293c23a59e24572f4b * Revert "fix(windows): skip presentation after close during restore" This reverts commit 2153d45. Agent-Session: https://cmux.com/deeplink/agent?id=aid-1a293c23a59e24572f4b * test(windows): a close the window refuses during the restore keeps it open Review of #27258 (BLOCK at 141a41b): noteUserClose recorded the close before the window's delegate decided, so a cancelled incognito confirmation during the restore left the window remembered as closed and the restore tore it down. That code is gone (base's #31155 records the close in windowWillClose, after AppKit carries it out); this test keeps it that way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-1a293c23a59e24572f4b * test(windows): read the open records outside #expect (a key path there throws) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-1a293c23a59e24572f4b --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
BackgroundAccessoryPolicyTests.aWindowClosedBeforeTheRestoreFinishesStaysClosedfailed 1 run in 3 on cmux12s (job 691124a2e9b4323980de7e07). It is a product race from the WindowManager restore (#23695), not a test fault: a window the person closed during the launch comes back.Cause: the restore creates its first workspace (
FirstWorkspaceLaunch.create). The create's reply can come before the store applies the event that lists the workspace (the store drains once per frame; the reply resumes the restore at once). The restore then ended and clearedclosedDuringRestoreandclosedLaunchWindowID(WindowManager.swift:246-249). The late report'sreconcileMembership(WindowManager+Membership.swift:497) found no window and no memory, registered a new fallback window and presented it.Fix: the memory of what the person closed during the restore is now one value next to
WindowRegistry,RestoreClosedWindows(CmuxNextLayout). The restore records the workspace it asked for that the store does not list yet, with the event sequence taken after the create's reply (DaemonConnection.eventSequence, a write barrier). When the person closed the launch window, that window is remembered past the restore: reconcile still gives a late workspace the closed launch window as its fallback and keeps it closed in the same registry step (no sync ever opens it), and the memory ends once each such workspace is in a window, or the store applied past the barrier without listing it (gone). Reconcile settles it before it picks the fallback, so a memory with nothing left to wait for never takes an unrelated workspace. The check is O(the restore's workspaces), runs only in reconcile, and adds no main-thread wait or action-path work.Test seam:
DaemonStore.eventHold(nil in the app) keeps buffered events unapplied while it returns true, and asks for no frame meanwhile;releaseHeldEvents()applies them in that turn. The new test holds the batch with the new workspace until the restore ended, so the failing order is certain, not a race.Testing
cmux-ci run --class light --display --script scripts/ci/package-test-lane.sh --ref <sha> --arg=suite --arg=Packages/macOS/CmuxNext --arg=<suites> --env CMUX_TEST_REQUIRE_GUI=1 --env CMUX_CI_XCODE_APP=/Applications/Xcode_26.6.appaWorkspaceTheRestoreAskedForReportedAfterItEndedStaysInTheClosedLaunchWindowfailed at BackgroundAccessoryPolicyTests.swift:170-172: a window was presented, one controller open, the workspace not in a closed window. Its preconditions held (the store listed nothing at the restore's end; the hold caught the report).RestoreClosedWindowsTestsonce, thenBackgroundAccessoryPolicyTests5 times in a row: all 6 suites passed (each run: the new test,aWindowClosedBeforeTheRestoreFinishesStaysClosed,noWindowIsAMenuBarApp…).Observed / Not run
Owner: no OWNERS.md row covers WindowManager (
CmuxNextApp/Windows/WindowManager*.swift),CmuxNextLayout/Windows/or the SwiftDaemonStore(not listed).Changelog
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes a race where a window the person closed while cmux was still launching could open again when the restore's first workspace was reported late. The closed window now stays closed.
The restore's first workspace create can reply before the store applies the event that lists the workspace; the restore then ended and forgot the closed window, so the late report's membership reconcile opened a new fallback window.
Bug Fixes
closedDuringRestoreandclosedLaunchWindowIDwithRestoreClosedWindows, which keeps the closed launch window remembered until the late workspaces land in a window or the store shows them gone.DaemonStore.eventHold/releaseHeldEvents()test seam and tests that hold the workspace report until the restore ends, making the failing order deterministic instead of a race.Written for commit ae9ad32. Summary will update on new commits.