Skip to content

Avoid beachball during update relaunch #4988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lawrencecchen wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Avoid beachball during update relaunch #4988

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43e0923
Add update relaunch snapshot policy tests
lawrencecchen May 29, 2026
ff8b446
Avoid full session snapshot during update relaunch
lawrencecchen May 29, 2026
0433c3c
Use bounded Ghostty tail reads for session snapshots
lawrencecchen May 29, 2026
3e71a96
Pin GhosttyKit checksum for bounded tail API
lawrencecchen May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 59 additions & 8 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
var shouldDeferInitialMainWindowBootstrapForExternalConfirmation = false
private var didBootstrapInitialMainWindow = false
private var isTerminatingApp = false
private var isUpdateRelaunchInProgress = false
private var didPersistUpdateRelaunchSnapshot = false
private var closedWindowHistorySuppressedWindowIds: Set<UUID> = []
#if DEBUG
var closeMainWindowContainingTabIdObserverForTesting: ((UUID, Bool) -> Void)?
Expand Down Expand Up @@ -1763,7 +1765,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
StartupBreadcrumbLog.append("appDelegate.willTerminate.begin")
isTerminatingApp = true
closeAllWebInspectorsBeforeAppTeardown()
_ = saveSessionSnapshotIncludingProcessDetectedIndexes(includeScrollback: true, removeWhenEmpty: false)
saveSessionSnapshotForAppTermination()
stopSessionAutosaveTimer()
CloudVMActionLauncher.shared.terminateAll()
CmuxSSHURLProcessLauncher.shared.terminateAll()
Expand Down Expand Up @@ -1792,7 +1794,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent

func persistSessionForUpdateRelaunch() {
isTerminatingApp = true
_ = saveSessionSnapshotIncludingProcessDetectedIndexes(includeScrollback: true, removeWhenEmpty: false)
isUpdateRelaunchInProgress = true
stopSessionAutosaveTimer()
didPersistUpdateRelaunchSnapshot = saveSessionSnapshot(
includeScrollback: false,
removeWhenEmpty: false,
forceSynchronousWrite: true
)
Comment on lines +1797 to +1801
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Show the snapshot entry point and how includeScrollback flows into the write decision.
ast-grep --pattern 'func saveSessionSnapshotIncludingProcessDetectedIndexes($$$) {
  $$$
}'

ast-grep --pattern 'func saveSessionSnapshot($$$) {
  $$$
}'

ast-grep --pattern 'static func shouldWriteSessionSnapshotSynchronously($$$) -> Bool {
  $$$
}'

# Inspect what includeScrollback triggers (e.g. bounded tail read vs full scrollback export).
rg -nP --type=swift -C4 '\bincludeScrollback\b'

# Confirm the relaunch call site runs on the main thread synchronously.
rg -nP --type=swift -C4 'persistSessionForUpdateRelaunch\s*\('

Repository: manaflow-ai/cmux

Length of output: 50373

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Inspect the exact relaunch snapshot code region.
echo "=== AppDelegate.swift around persistSessionForUpdateRelaunch ==="
sed -n '1765,1825p' Sources/AppDelegate.swift

# 2) Locate updaterWillRelaunchApplication call path and where persistSessionForUpdateRelaunch is invoked.
echo "=== Find updaterWillRelaunchApplication ==="
rg -n "updaterWillRelaunchApplication" -S Sources

echo "=== Find persistSessionForUpdateRelaunch call sites ==="
rg -n "persistSessionForUpdateRelaunch\\s*\\(" -S Sources

# If UpdateDelegate.swift exists, inspect the likely area mentioned in the review comment.
if [ -f Sources/UpdateDelegate.swift ]; then
  echo "=== UpdateDelegate.swift: around 80-170 ==="
  sed -n '80,170p' Sources/UpdateDelegate.swift
fi

# 3) Inspect snapshot write-policy logic directly.
echo "=== shouldWriteSessionSnapshotSynchronously ==="
sed -n '4000,4060p' Sources/AppDelegate.swift

# 4) Confirm includeScrollback propagates into build snapshot for the relaunch snapshot helper.
echo "=== saveSessionSnapshotIncludingProcessDetectedIndexes / saveSessionSnapshot ==="
rg -n "saveSessionSnapshotIncludingProcessDetectedIndexes\\(" -S Sources/AppDelegate.swift
sed -n '3950,4020p' Sources/AppDelegate.swift

# 5) Find any existing comment about relaunch snapshot being minimal/no-scrollback or with scrollback.
echo "=== Search for 'relaunch' + 'scrollback' ==="
rg -n "relaunch.*scrollback|scrollback.*relaunch" -S Sources/AppDelegate.swift Sources

Repository: manaflow-ai/cmux

Length of output: 8910

🏁 Script executed:

#!/bin/bash
# (placeholder, only one shell block should be used)

Repository: owner/repo

Length of output: 2026

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== UpdateDelegate.swift: updaterWillRelaunchApplication ==="
sed -n '70,160p' Sources/Update/UpdateDelegate.swift

echo "=== saveSessionSnapshot: sync vs async write behavior ==="
sed -n '3685,3785p' Sources/AppDelegate.swift

echo "=== persistSessionSnapshot write helper (if present) ==="
rg -n "persistSessionSnapshot\\(" -S Sources/AppDelegate.swift
# show the first match region around the definition
rg -n "private func persistSessionSnapshot\\(" -S Sources/AppDelegate.swift && sed -n '3785,3920p' Sources/AppDelegate.swift

Repository: manaflow-ai/cmux

Length of output: 12336

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== persistSessionSnapshot implementation ==="
# show around first definition
rg -n "private func persistSessionSnapshot\\(" Sources/AppDelegate.swift
sed -n '4070,4180p' Sources/AppDelegate.swift

echo "=== Find where 'synchronously:' is used/checked ==="
rg -n "synchronously:\\s*|synchronously\\b" Sources/AppDelegate.swift -C2

echo "=== Search for bounded tail / scrollback export behavior tied to includeScrollback ==="
rg -n "bounded|tail|scrollback" Sources/AppDelegate.swift -S -g'*.swift' -C2

# Also search for bounded tail in the broader codebase for snapshot persistence logic.
rg -n "bounded.*tail|tail.*read|scrollback.*bounded|includeScrollback" Sources -S -g'*.swift' -C2 | head -n 80

Repository: manaflow-ai/cmux

Length of output: 25784

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== SessionPersistencePolicy scrollback limits ==="
rg -n "maxScrollbackLinesPerTerminal|SessionPersistencePolicy" Sources/SessionPersistencePolicy.swift Sources -S -g'*.swift' -C1

# If SessionPersistencePolicy is in another file, the rg above will include it; also directly search:
rg -n "maxScrollbackLinesPerTerminal" Sources -S -g'*.swift' -C3

echo "=== Workspace.swift: capturedScrollback gating (shouldPersistScrollback, hibernationState) ==="
sed -n '520,640p' Sources/Workspace.swift
rg -n "shouldPersistScrollback" Sources/Workspace.swift -S -C3

echo "=== Workspace.swift: TerminalController.shared.readTerminalTextForSnapshot arguments ==="
sed -n '540,610p' Sources/Workspace.swift

echo "=== Any comments describing update relaunch snapshot goal (minimal/no-scrollback) ==="
rg -n "update relaunch|relaunch.*session|persistSessionForUpdateRelaunch|minimal.*scrollback|no scrollback" Sources/AppDelegate.swift Sources -S -g'*.swift' -C2

Repository: manaflow-ai/cmux

Length of output: 11536

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Workspace.swift: scrollback capture gating around capturedScrollback ==="
sed -n '540,620p' Sources/Workspace.swift

echo "=== Workspace.swift: shouldPersistScrollback definition/usage in this file ==="
rg -n "shouldPersistScrollback" Sources/Workspace.swift -S
# print a bit of the surrounding definitions
rg -n "shouldPersistScrollback" Sources/Workspace.swift -S -C3

echo "=== SessionPersistence.swift: scrollback limits and truncation helpers ==="
sed -n '1,120p' Sources/SessionPersistence.swift

echo "=== SessionPersistence.swift: truncatedScrollback implementation ==="
rg -n "truncatedScrollback" Sources/SessionPersistence.swift -S
rg -n "truncatedScrollback\\(" Sources/SessionPersistence.swift -S -C2 | head -n 40

# show the specific implementation region (best-effort by locating a few line ranges)
# (Using approximate range; if too small/large, re-run with adjusted bounds.)
sed -n '150,260p' Sources/SessionPersistence.swift

echo "=== Any comments/logic in Workspace about update relaunch minimal/no scrollback ==="
rg -n "update relaunch|update.*relaunch|relaunch.*scrollback|minimal.*scrollback|no scrollback" Sources/Workspace.swift Sources/AppDelegate.swift Sources/Update/UpdateDelegate.swift -S -g'*.swift'

Repository: manaflow-ai/cmux

Length of output: 15512

Fix update-relaunch session snapshot: includeScrollback: true persists scrollback in a main-thread synchronous save

persistSessionForUpdateRelaunch() sets isTerminatingApp = true and calls saveSessionSnapshotIncludingProcessDetectedIndexes(includeScrollback: true, ..., forceSynchronousWrite: true). updaterWillRelaunchApplication(_:) invokes this on the main thread (directly if already on main, otherwise via DispatchQueue.main.sync). With forceSynchronousWrite: true, saveSessionSnapshot(...) performs an immediate write (persistSessionSnapshot(..., synchronously: true) runs writeBlock() inline).

Because includeScrollback is true, snapshot building captures scrollback (bounded) via TerminalController.shared.readTerminalTextForSnapshot(..., includeScrollback: true, lineLimit: SessionPersistencePolicy.maxScrollbackLinesPerTerminal) when shouldPersistScrollback && shouldReplaySessionScrollback && hibernationState == nil. This means the relaunch snapshot is not “no scrollback” (even though it’s truncated to policy limits).

If the update-relaunch path is intended to be “minimal, no scrollback”, switch includeScrollback to false (or ensure the policy conditions match that goal).

🐢 Proposed change if scrollback should be excluded on the relaunch path 
         didPersistUpdateRelaunchSnapshot = saveSessionSnapshotIncludingProcessDetectedIndexes(
-            includeScrollback: true,
+            includeScrollback: false,
             removeWhenEmpty: false,
             forceSynchronousWrite: true
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
didPersistUpdateRelaunchSnapshot = saveSessionSnapshotIncludingProcessDetectedIndexes(
includeScrollback: true,
removeWhenEmpty: false,
forceSynchronousWrite: true
)
didPersistUpdateRelaunchSnapshot = saveSessionSnapshotIncludingProcessDetectedIndexes(
includeScrollback: false,
removeWhenEmpty: false,
forceSynchronousWrite: true
)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/AppDelegate.swift` around lines 1797 - 1801,
persistSessionForUpdateRelaunch() currently calls
saveSessionSnapshotIncludingProcessDetectedIndexes(includeScrollback: true,
forceSynchronousWrite: true) which causes
TerminalController.shared.readTerminalTextForSnapshot(..., includeScrollback:
true, ...) to capture scrollback synchronously during
updaterWillRelaunchApplication(_:). To make the relaunch snapshot minimal (no
scrollback), change the includeScrollback argument to false in the
saveSessionSnapshotIncludingProcessDetectedIndexes call inside
persistSessionForUpdateRelaunch(), or alternatively adjust the session
persistence policy checks (shouldPersistScrollback /
shouldReplaySessionScrollback / hibernationState) so that
readTerminalTextForSnapshot is not invoked with includeScrollback=true on the
update-relaunch path.

}

func configure(tabManager: TabManager, notificationStore: TerminalNotificationStore, sidebarState: SidebarState) {
Expand Down Expand Up @@ -3687,7 +3695,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
includeScrollback: Bool,
removeWhenEmpty: Bool = false,
restorableAgentIndex: RestorableAgentSessionIndex? = nil,
surfaceResumeBindingIndex: SurfaceResumeBindingIndex? = nil
surfaceResumeBindingIndex: SurfaceResumeBindingIndex? = nil,
forceSynchronousWrite: Bool = false
) -> Bool {
if Self.shouldSkipSessionSaveDuringRestore(
isApplyingSessionRestore: isApplyingSessionRestore,
Expand All @@ -3700,7 +3709,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
}
let writeSynchronously = Self.shouldWriteSessionSnapshotSynchronously(
isTerminatingApp: isTerminatingApp,
includeScrollback: includeScrollback
includeScrollback: includeScrollback,
forceSynchronousWrite: forceSynchronousWrite
)
if writeSynchronously {
TextBoxInputTextView.flushPendingSessionDraftAttachmentCopies()
Expand Down Expand Up @@ -3958,14 +3968,41 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
@discardableResult
private func saveSessionSnapshotIncludingProcessDetectedIndexes(
includeScrollback: Bool,
removeWhenEmpty: Bool = false
removeWhenEmpty: Bool = false,
forceSynchronousWrite: Bool = false
) -> Bool {
let resumeIndexes = ProcessDetectedResumeIndexes.loadSynchronously()
return saveSessionSnapshot(
includeScrollback: includeScrollback,
removeWhenEmpty: removeWhenEmpty,
restorableAgentIndex: resumeIndexes.restorableAgentIndex,
surfaceResumeBindingIndex: resumeIndexes.surfaceResumeBindingIndex
surfaceResumeBindingIndex: resumeIndexes.surfaceResumeBindingIndex,
forceSynchronousWrite: forceSynchronousWrite
)
}

private func saveSessionSnapshotForAppTermination() {
guard Self.shouldSaveSessionSnapshotOnApplicationTerminate(
didPersistUpdateRelaunchSnapshot: didPersistUpdateRelaunchSnapshot
) else {
StartupBreadcrumbLog.append("appDelegate.willTerminate.snapshotSkipped.updateRelaunch")
return
}

if Self.shouldUseFastSessionSnapshotForUpdateRelaunch(
isUpdateRelaunchInProgress: isUpdateRelaunchInProgress
) {
_ = saveSessionSnapshot(
includeScrollback: false,
removeWhenEmpty: false,
forceSynchronousWrite: true
)
return
}
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Update-relaunch fallback snapshot silently drops ProcessDetectedResumeIndexes

When persistSessionForUpdateRelaunch ran but the synchronous write returned false (didPersistUpdateRelaunchSnapshot == false) while isUpdateRelaunchInProgress == true, saveSessionSnapshotForAppTermination falls into the fast-path branch that calls saveSessionSnapshot(includeScrollback:false, ...) directly — skipping the ProcessDetectedResumeIndexes loaded by saveSessionSnapshotIncludingProcessDetectedIndexes. The normal termination path below it does load those indexes. If saveSessionSnapshot fails during persistSessionForUpdateRelaunch (e.g., snapshot builder returns nil), the termination snapshot will be written without the restorable-agent and surface-resume-binding indexes, and the subsequent launch may restore into a degraded state. Consider whether the fallback path here should also load process-detected indexes, or whether the fast path is truly sufficient when the first write attempt failed.

Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)


_ = saveSessionSnapshotIncludingProcessDetectedIndexes(
includeScrollback: true,
removeWhenEmpty: false
)
}

Expand Down Expand Up @@ -4004,9 +4041,23 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent

nonisolated static func shouldWriteSessionSnapshotSynchronously(
isTerminatingApp: Bool,
includeScrollback: Bool
includeScrollback: Bool,
forceSynchronousWrite: Bool = false
) -> Bool {
if forceSynchronousWrite { return true }
return isTerminatingApp && includeScrollback
}

nonisolated static func shouldSaveSessionSnapshotOnApplicationTerminate(
didPersistUpdateRelaunchSnapshot: Bool
) -> Bool {
!didPersistUpdateRelaunchSnapshot
}

nonisolated static func shouldUseFastSessionSnapshotForUpdateRelaunch(
isUpdateRelaunchInProgress: Bool
) -> Bool {
isTerminatingApp && includeScrollback
isUpdateRelaunchInProgress
}

nonisolated static func shouldSkipSessionAutosaveForUnchangedFingerprint(
Expand Down
20 changes: 14 additions & 6 deletions Sources/Update/UpdateDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,22 @@ extension UpdateDriver: SPUUpdaterDelegate {
}

func updaterWillRelaunchApplication(_ updater: SPUUpdater) {
Task { @MainActor in
AppDelegate.shared?.persistSessionForUpdateRelaunch()
TerminalController.shared.stop()
NSApp.invalidateRestorableState()
for window in NSApp.windows {
window.invalidateRestorableState()
let prepareForRelaunch = {
MainActor.assumeIsolated {
AppDelegate.shared?.persistSessionForUpdateRelaunch()
TerminalController.shared.stop()
NSApp.invalidateRestorableState()
for window in NSApp.windows {
window.invalidateRestorableState()
}
}
}

if Thread.isMainThread {
prepareForRelaunch()
} else {
DispatchQueue.main.sync(execute: prepareForRelaunch)
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 DispatchQueue.main.sync is a blocking primitive

DispatchQueue.main.sync is explicitly flagged by the cmux blocking-runtime rule. When Sparkle calls updaterWillRelaunchApplication from a background thread, this blocks that thread until all of the main-thread work — including persistSessionForUpdateRelaunch's synchronous file write — completes. If Sparkle holds any lock that the main thread is waiting to acquire (e.g., via another main.sync or an internal Sparkle XPC barrier), this produces a deadlock. The fix replaced a race with a hard block, but DispatchQueue.main.sync is still a timing-based synchronization primitive.

A conforming alternative is to check Sparkle's documented thread-delivery guarantee for updaterWillRelaunchApplication. If Sparkle guarantees main-thread delivery (or if the SPUStandardUpdaterController setup forces it), the Thread.isMainThread fast path already handles the call and the DispatchQueue.main.sync branch is dead code that can be removed entirely. If it is not guaranteed, a RunLoop.main.run(until:) spin or a dedicated Sparkle-owned notification fired on the main thread would be a safer coordination point.

Rule Used: Flag new blocking or timing-based synchronization ... (source)

}
}
}

Expand Down
30 changes: 30 additions & 0 deletions cmuxTests/SessionPersistenceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,36 @@ final class SessionPersistenceTests: XCTestCase {
includeScrollback: true
)
)
XCTAssertTrue(
AppDelegate.shouldWriteSessionSnapshotSynchronously(
isTerminatingApp: false,
includeScrollback: false,
forceSynchronousWrite: true
)
)
}

func testUpdateRelaunchUsesFastTerminationSnapshotPolicy() {
XCTAssertFalse(
AppDelegate.shouldSaveSessionSnapshotOnApplicationTerminate(
didPersistUpdateRelaunchSnapshot: true
)
)
XCTAssertTrue(
AppDelegate.shouldSaveSessionSnapshotOnApplicationTerminate(
didPersistUpdateRelaunchSnapshot: false
)
)
XCTAssertTrue(
AppDelegate.shouldUseFastSessionSnapshotForUpdateRelaunch(
isUpdateRelaunchInProgress: true
)
)
XCTAssertFalse(
AppDelegate.shouldUseFastSessionSnapshotForUpdateRelaunch(
isUpdateRelaunchInProgress: false
)
)
}

func testRestoreCompletionSavePolicySkipsManualReopen() {
Expand Down
Loading