Avoid beachball during update relaunch#4988
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR replaces the pasteboard/temp-file VT export path for terminal scrollback snapshots with a new bounded Ghostty C API (
Confidence Score: 4/5Safe to merge with attention to the VIEWPORT fallback format inconsistency in readTerminalTextForSnapshot. The core update-relaunch flow — synchronous snapshot write, autosave-timer stop, and duplicate-snapshot skip — is well-structured and backed by new unit tests. The two findings are both in the terminal text read path: the VIEWPORT fallback silently ignores allowVTExport (inconsistent format on API failure), and readTerminalTextBase64's scrollback path omits the byte cap that readTerminalTextForSnapshot correctly applies. Neither affects the primary beachball fix, but the format inconsistency could cause degraded session restore fidelity in the error path. Sources/TerminalController.swift — the readTerminalTextForSnapshot fallback and readTerminalTextBase64 scrollback path. Important Files Changed
Sequence DiagramsequenceDiagram
participant Sparkle
participant UpdateDelegate
participant MainThread as Main Thread
participant AppDelegate
participant TerminalController
participant GhosttyAPI as Ghostty C API
Sparkle->>UpdateDelegate: updaterWillRelaunchApplication()
Note over UpdateDelegate: Thread.isMainThread ?
alt already on main thread
UpdateDelegate->>MainThread: prepareForRelaunch() directly
else background thread
UpdateDelegate->>MainThread: "DispatchQueue.main.sync { prepareForRelaunch() }"
end
MainThread->>AppDelegate: persistSessionForUpdateRelaunch()
AppDelegate->>AppDelegate: "isTerminatingApp = true"
AppDelegate->>AppDelegate: stopSessionAutosaveTimer()
AppDelegate->>AppDelegate: saveSessionSnapshotIncludingProcessDetectedIndexes(forceSynchronousWrite: true)
AppDelegate->>GhosttyAPI: ghostty_surface_read_text_tail(scope: SCREEN, max_bytes: limit)
GhosttyAPI-->>AppDelegate: bounded text
AppDelegate->>AppDelegate: write snapshot (synchronous)
AppDelegate->>AppDelegate: "didPersistUpdateRelaunchSnapshot = true"
MainThread->>TerminalController: stop()
MainThread->>MainThread: NSApp.invalidateRestorableState()
Sparkle->>Sparkle: relaunch app
Note over AppDelegate: applicationWillTerminate fires
AppDelegate->>AppDelegate: saveSessionSnapshotForAppTermination()
AppDelegate->>AppDelegate: "shouldSave? didPersistUpdateRelaunchSnapshot == true → SKIP"
Note over AppDelegate: No duplicate snapshot written
Reviews (3): Last reviewed commit: "Pin GhosttyKit checksum for bounded tail..." | Re-trigger Greptile |
📝 WalkthroughWalkthroughSession snapshot persistence was refactored to track update-relaunch snapshots and support forced synchronous writes. AppDelegate gained a relaunch-persisted flag and conditional termination-snapshot logic. UpdateDelegate relaunch prep was made synchronous. Terminal snapshot export now reads bounded tails via ghostty. Tests and the ghostty submodule were updated. ChangesUpdate Relaunch and Termination Snapshot Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if Thread.isMainThread { | ||
| prepareForRelaunch() | ||
| } else { | ||
| DispatchQueue.main.sync(execute: prepareForRelaunch) |
There was a problem hiding this comment.
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)
| if Self.shouldUseFastSessionSnapshotForUpdateRelaunch( | ||
| isUpdateRelaunchInProgress: isUpdateRelaunchInProgress | ||
| ) { | ||
| _ = saveSessionSnapshot( | ||
| includeScrollback: false, | ||
| removeWhenEmpty: false, | ||
| forceSynchronousWrite: true | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
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)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| scope: GHOSTTY_TEXT_SCOPE_SCREEN, | ||
| format: allowVTExport ? GHOSTTY_TEXT_FORMAT_VT : GHOSTTY_TEXT_FORMAT_PLAIN, | ||
| lineLimit: lineLimit, | ||
| byteLimit: SessionPersistencePolicy.maxScrollbackCharactersPerTerminal |
There was a problem hiding this comment.
Character-count constant misused as byte limit
Low Severity
SessionPersistencePolicy.maxScrollbackCharactersPerTerminal (400,000) is a character-count constant, used with text.count in truncatedScrollback. Here it's passed as byteLimit, which maps to max_bytes in the ghostty C API. For multi-byte UTF-8 content (e.g., CJK text), bytes-per-character can be 3–4×, so the effective limit drops to ~100K–133K characters instead of the intended 400K, silently truncating more scrollback than expected for non-ASCII terminal sessions.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0433c3c. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3e71a96. Configure here.
| format: GHOSTTY_TEXT_FORMAT_PLAIN, | ||
| lineLimit: lineLimit, | ||
| byteLimit: SessionPersistencePolicy.maxScrollbackCharactersPerTerminal | ||
| ) |
There was a problem hiding this comment.
Fallback loses scrollback by switching to viewport scope
Medium Severity
When includeScrollback is true, the first readTerminalTextTail call uses GHOSTTY_TEXT_SCOPE_SCREEN with VT format. If that returns nil, the fallback switches to GHOSTTY_TEXT_SCOPE_VIEWPORT (plain format), which only captures the visible portion and discards all scrollback history. If the failure was caused by the VT format rather than the screen scope, falling back to screen/plain would preserve scrollback. The current fallback undermines the purpose of includeScrollback: true for the session snapshot persistence path.
Reviewed by Cursor Bugbot for commit 3e71a96. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 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.
In `@Sources/TerminalController.swift`:
- Around line 10051-10052: The code passes
SessionPersistencePolicy.maxScrollbackCharactersPerTerminal (a character-count
limit) into readTerminalTextTail as byteLimit, causing character/byte unit
mismatch; change the argument to the matching byte-based policy
(SessionPersistencePolicy.maxScrollbackBytesPerTerminal) or convert the
character limit to bytes before calling readTerminalTextTail so the units align;
update the call site of readTerminalTextTail in TerminalController (the
byteLimit parameter) to use the byte-based constant (or a utf8.count conversion
from the character limit) so persistence/snapshot truncation behavior matches
SessionPersistencePolicy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8252ab8d-a77d-40b8-811e-f8cfed863b9e
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/TerminalController.swiftcmuxTests/SessionPersistenceTests.swiftghostty
| didPersistUpdateRelaunchSnapshot = saveSessionSnapshotIncludingProcessDetectedIndexes( | ||
| includeScrollback: true, | ||
| removeWhenEmpty: false, | ||
| forceSynchronousWrite: true | ||
| ) |
There was a problem hiding this comment.
🧩 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 SourcesRepository: 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.swiftRepository: 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 80Repository: 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' -C2Repository: 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.
| 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.
| lineLimit: lineLimit, | ||
| byteLimit: SessionPersistencePolicy.maxScrollbackCharactersPerTerminal |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check SessionPersistencePolicy definition for maxScrollbackCharactersPerTerminal
# Find the policy definition
ast-grep --pattern 'maxScrollbackCharactersPerTerminal'
# Check for any documentation or comments explaining the unit
rg -B3 -A3 'maxScrollbackCharactersPerTerminal'Repository: manaflow-ai/cmux
Length of output: 6297
Fix scrollback truncation unit mismatch (characters vs bytes)
SessionPersistencePolicy.maxScrollbackCharactersPerTerminal is defined and enforced as a character-count limit (text.count) in Sources/SessionPersistence.swift, but Sources/TerminalController.swift passes it to readTerminalTextTail as byteLimit. This mixes units (characters vs bytes) and will truncate fewer characters than intended for UTF-8; align the policy/unit with the parameter (maxScrollbackBytesPerTerminal vs character-bounded limit) so persistence/snapshot truncation match.
🤖 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/TerminalController.swift` around lines 10051 - 10052, The code passes
SessionPersistencePolicy.maxScrollbackCharactersPerTerminal (a character-count
limit) into readTerminalTextTail as byteLimit, causing character/byte unit
mismatch; change the argument to the matching byte-based policy
(SessionPersistencePolicy.maxScrollbackBytesPerTerminal) or convert the
character limit to bytes before calling readTerminalTextTail so the units align;
update the call site of readTerminalTextTail in TerminalController (the
byteLimit parameter) to use the byte-based constant (or a utf8.count conversion
from the character limit) so persistence/snapshot truncation behavior matches
SessionPersistencePolicy.
1 participant
Summary
Notes
Verification
git diff --checkgit -C ghostty diff --check./tests/test_ci_ghosttykit_checksum_present.sh./scripts/reload.sh --tag updhangNote
Medium Risk
Changes session persistence and terminal scrollback capture at quit/update relaunch; incorrect behavior could lose or truncate restored sessions, but scope is bounded and covered by new policy tests.
Overview
Fixes update relaunch UI hangs by making Sparkle’s relaunch prep run synchronously on the main thread and persisting the session snapshot with a forced synchronous write before quit.
Session snapshot scrollback now uses Ghostty’s
read_text_tailAPI (plain/VT, byte-capped) instead of merging selection tags and the old pasteboard/temp-file VT export path.applicationWillTerminateskips a second full snapshot when update relaunch already saved one.Bumps ghosttykit to the fork commit that adds the text-tail API; tests cover forced sync writes and duplicate termination skip.
Reviewed by Cursor Bugbot for commit 3e71a96. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit