fix: keep closed windows closed across relaunch - #363
Conversation
Since #336 an ordinary window close hides the window and keeps its workspaces and shells registered so the Dock can reopen it. The session snapshot wrote that hidden window like a visible one, so every relaunch restored an extra window of stale workspaces and re-saved it, forever. Snapshots now flag hidden windows and sort them last. Startup restore skips them, retrieves each of their escrowed shells from the holder, hangs it up, removes its WAL directory, and tells the orphan reconciler to ignore those ids so they cannot come back as a recovery window.
![darkestdarky-bot[bot]](https://avatars.githubusercontent.com/in/3783368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diff correctly fixes the bug where closed windows reappeared on relaunch by adding an isHidden flag to SessionWindowSnapshot, filtering hidden windows from restore, and ending their escrowed shells at startup. The implementation is thorough: it sorts hidden windows last in snapshots, skips their scrollback, excludes their session IDs from orphan reconciliation, and includes unit tests for both the new flag and backward compatibility with older snapshots. The approach is sound and the test plan covers the key scenarios.
Review coverage: 304/304 diff lines supplied. Partial input (truncated: standards). Inline comments are limited to fully visible, valid right-side hunks. Reviewed commit: 290882bfb1a681e228b3d1e0cd73f182b930105f.
| display: displaySnapshot(for: window), | ||
| tabManager: context.tabManager.sessionSnapshot(includeScrollback: includeScrollback), | ||
| // A hidden window is never shown again, so its scrollback is dead weight. | ||
| tabManager: context.tabManager.sessionSnapshot(includeScrollback: includeScrollback && !isHidden), |
There was a problem hiding this comment.
The isHidden: isHidden ? true : nil pattern is intentional for Codable optional encoding (nil omits the key, true writes it), but the computed property isHiddenWindow already handles nil-as-false. Consider adding a brief comment explaining why true : nil is used instead of just isHidden, since the ternary with identical values looks like a no-op at first glance.
| let socketPath = meta.escrowSocketPath, | ||
| let tokenHex = meta.escrowToken, | ||
| let masterFD = SessionEscrowClient.retrieve( | ||
| sessionId: sessionId, |
There was a problem hiding this comment.
endShellsOfHiddenWindows calls kill(childPID, SIGHUP) and close(masterFD) without checking the return values. While the function logs how many sessions were ended, a failed kill or close is silently counted as success. Consider logging individual failures so a hung process or bad fd doesn't go unnoticed.
| tokenHex: tokenHex, | ||
| socketPath: socketPath | ||
| ) { | ||
| if let childPID = meta.childPID, childPID > 0 { |
There was a problem hiding this comment.
SessionWALStore.shared.discardOrphanedSession(sessionId:force:true) is called unconditionally after the retrieve block, even when retrieve returned nil. This means a session whose holder refused to hand over the fd still gets its WAL directory force-removed. The comment on startupEndedHiddenSessionIds says "a session the holder refused to hand over must not be revived either," which justifies the removal, but the data loss of a potentially recoverable session should be explicitly noted in a comment here.
| /// registered but ordered out). Restore never shows a hidden window again: it ends the | ||
| /// window's escrowed shells instead, so a closed window stays closed across a relaunch. | ||
| /// `nil` for snapshots written before this field existed -- treat as visible. | ||
| var isHidden: Bool? |
There was a problem hiding this comment.
The isHidden: Bool? field uses a nil-means-false convention for backward compatibility, which is clean. The computed property isHiddenWindow correctly coalesces to false. This is a good pattern, but consider whether var isHidden: Bool = false with a custom decoding init that defaults missing keys to false would be simpler and avoid optional unwrapping at every call site.
| notifyUncleanShutdownRecovery() | ||
| } | ||
| let primaryWindowSnapshot = startupSnapshot?.windows.first | ||
| // Windows the user had closed before quitting are not shown again: their escrowed |
There was a problem hiding this comment.
The windowsToRestore array is computed with optional binding on startupSnapshot, but then startupSnapshot is force-unwrapped in the if let startupSnapshot block on line 1798. The double optional binding is redundant—the second if let could just use the already-bound value from line 1796, or the first binding could be restructured to avoid the repetition.
| .windows | ||
| .dropFirst() | ||
| .prefix(max(0, SessionPersistencePolicy.maxWindowsPerSnapshot - 1))) | ||
| if startupSnapshot != nil { |
There was a problem hiding this comment.
The additionalWindows array no longer applies prefix(max(0, SessionPersistencePolicy.maxWindowsPerSnapshot - 1)) after the refactor. The filtering in windowsToRestore(from:) already applies the limit, so this is correct, but the removal of the explicit prefix here means the limit is now enforced only inside SessionPersistenceStore. This is fine since windowsToRestore is the single source of truth, but worth noting that the old defensive clamp is gone.
| let snapshot = try XCTUnwrap(appDelegate.buildSessionSnapshot(includeScrollback: false)) | ||
| XCTAssertEqual(snapshot.windows.count, 2) | ||
| let visible = try XCTUnwrap(snapshot.windows.first) | ||
| let hidden = try XCTUnwrap(snapshot.windows.last) |
There was a problem hiding this comment.
The test calls closedWindow.performClose(nil) and then asserts XCTAssertFalse(closedWindow.isVisible). This depends on preserveMainWindowOnClose keeping the window registered but hidden. If that behavior changes, this test will break. Consider adding a comment referencing the preserveMainWindowOnClose mechanism so future maintainers understand the dependency.
1 participant
What this does
Closing a Programa window with the red button no longer brings that window back on the next launch. Since 0.5.0 (PR #336) every window you ever closed came back on restart as an extra window full of stale workspaces, and closing it again did not help.
Summary
PR #336 made an ordinary window close hide the window and keep its workspaces and shells registered so the Dock can reopen it in the same run.
buildSessionSnapshotwrote that hidden window exactly like a visible one, and startup restore turned it into a normal visible window, which was then re-saved as visible. In the reporter's production session file the second window had been carried through every snapshot since Sep 16 with the same 6 empty shells.Changes:
SessionWindowSnapshot.isHidden(optional, old snapshots decode as visible).buildSessionSnapshotsets it fromcontext.hiddenWindow, sorts hidden windows last so the primary restore entry is always a visible one, and skips their scrollback.SessionPersistenceStore.windowsToRestoredrops hidden windows;hiddenWindows(from:)returns them. Both the startup restore and the socket archive restore use it.AppDelegate.endShellsOfHiddenWindowsruns at startup before any window restore: for each terminal panel of a hidden window it retrieves the escrowed pty from the holder like a reattach, sends SIGHUP to the child, closes the fd, and force-removes the WAL directory. Those ids are also excluded from the orphan reconciler so they cannot resurface as a recovery window.Review order:
SessionPersistence.swift, thenAppDelegate+SessionSnapshotPersistence.swift, thenAppDelegate.swift.Test plan
testSessionSnapshotFlagsClosedWindowHiddenAndOrdersItLast(native close, snapshot flags it hidden and last,windowsToRestoredrops it, Dock reopen clears the flag) andtestWindowsToRestoreSkipsHiddenWindowsAndOlderSnapshotsStayVisible(filter plus decode of a snapshot without the key). Both compile locally; CI runs them.session.restore hiddenWindows=1 sessions=6 ended=0.isHidden: true, shell still alive), quit, relaunch: one window,hiddenWindows=1 sessions=1 ended=1, the shell's tty and WAL directory are gone,escrow.reconcile recovered=0.