Skip to content

cmux NEXT: a keep quit ends within 4 s whatever any step does; Restart to Update never leaves cmux invisible - #22919

Merged
lawrencecchen merged 24 commits into
feat-cmux-nextfrom
feat-cmux-next-slowquit
Oct 31, 2026
Merged

lawrencecchen merged 24 commits into
feat-cmux-nextfrom
feat-cmux-next-slowquit

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Incident 2026-10-20 (nightly 3951982493001): Lawrence clicked Restart to Update. The app looked frozen. Process 5342 exited 28 s after the click; launch looked slow as well. Bead cx-p00l3.

Root cause (unified log, log show of pid 5342 and the Sparkle tools):

Time Event
19:28:43.545 Click. Every window goes transparent (UpdateWindowHandoff.hide at willInstallStaged).
19:28:50.736 Windows come back: Sparkle aborted the install after its 7 s first-installer-message timeout (SPUInstallerDriver FIRST_INSTALLER_MESSAGE_TIMEOUT, "An error occurred while starting the installer"). The installer Autoupdate[6160] had staged the update at 19:00:14 and logged nothing in this window.
19:29:02.48 A click opens a URL in the default browser (Download Manually on the failure sheet).
19:29:09.8 A click (Retry).
19:29:10.971 Handling Quit AppleEvent (Sparkle's agent).
19:29:11.257 CEF shutdown complete, replyToApplicationShouldTerminate:YES. The quit itself took 0.29 s.
19:29:11.46 New app spawned; window on screen at 19:29:13.6.
19:29:13 to 19:29:33 New cmux-tui daemon (version handoff) adopts 113 terminal hosts one by one before it answers; terminals blank until then.

So the 28 s was not the quit: it was 7 s of no visible windows, then the failure sheet and a second click. The launch delay is the daemon's serial host adoption (follow-up cx-p00l3.1).

Changes

  • Restart to Update: the windows hide at updaterWillRelaunchApplication (Sparkle calls it only on the path that sends its installer to stage 2 and quits the app), not at the click, and only for a relaunch the person asked for (hidesForRelaunch, called when no automatic install is in flight). A click whose install fails keeps the windows on screen with the Installing state, then the failure. An install nobody clicked (Sparkle's automatic driver, the relaunch behind the person's app) leaves the windows as they are, as before; a declined relaunch behind and an automatic failure still call installAbandoned. The willInstallStaged hook, which nothing set any more, is gone.
  • Quit hard bound (keep quits): a quit that keeps sessions ends 4 s after it is decided whatever any step does (QuitDeadline.keepQuitLimit). An End quit has no deadline until its engines begin to stop (below). Each save step has a limit (QuitSaveSteps in CmuxNextControl, run by a QuitStepLimiter instance that QuitCoordinator owns): browser records 300 ms, windows 500 ms, sidebar 300 ms, Home's cache 400 ms, agent chat composers 400 ms, then terminal frames, agent images and remote snapshots 200 ms each. The person's own data saves first. Cloud's quit work (Codex broker stop, VM credential revokes) starts first and runs beside the saves, so a slow network never delays a save. If every step hangs they take 2.6 s (worstCase; Cloud's in-step wait is 100 ms, the late-step wait gives it the rest); with the 500 ms quit mark (QuitDeadline.quitMarkLimit), the 300 ms "Don't ask again" write and the 200 ms reply margin, 400 ms remain.
  • Late steps still finish: a step past its limit keeps running beside the next steps, and the quit waits for every such step before AppKit's reply, for what is left of its deadline less the 200 ms reply margin (QuitStepLimiter.finishOverruns, QuitDeadline.timeLeft; async, never a main-thread wait). An End quit also waits up to 2 s for a late save (not Cloud's step) before its sessions and daemon stop. In a keep quit, late saves get what the deadline leaves less the engines' 2 s close wait and the reply margin, so Chromium still closes its pages and flushes cookies and storage; a save still late then runs on beside the engine stop, and the wait before AppKit's reply covers it. A closed page has no session to save (BrowserRestoredSession), so a late browser-records save writes nothing over the good record the daemon holds (written after each navigation). In an End quit the deadline is armed only as the engines stop: a save still late after its 2 s End wait runs on beside the engine stop (its daemon has stopped). Chat pages are asked for their composer state within 1 s (AgentPaneRelaunchWake.askLimit, as on feat): the page first waits up to 500 ms for its typed draft to reach acpmux (draftSettleMs in schemas/agent-pane/wake-state.json, which composerDraft.ts and the host's test both read), and the quit waits for the composer step past its 400 ms limit. A page that does not answer in time is logged (agent-pane.relaunch-wake). Engines' close wait 2 s (was 3 s); an End quit ends 3 s after its engines begin to stop (was 15 s). The window records flush before the incognito close, and a windows step past its limit stops saving at once. Each step writes a quit.* launch mark.
  • Diagnostics: the quit origin and choice log at notice level, so they stay on disk. The updater log (where the incident's Sparkle error text was, lost because it was info level) moves to notice level in the separate high-tier PR cmux NEXT updater: the update log stays on disk (notice level) #22920.
  • Bench script (not CI): scripts/cmux-next/quit-launch-bound.py (behavior check: real tagged app, control socket, launch-mark pipe, loopback pages with beforeunload handlers, sample of every quit and launch). It fails on a quit over 6 s, a launch over 3 s to the first window or 8 s to the first live terminal frame (from main()), a launch main-thread stall over 2 s, or a restored session with fewer workspaces or tabs than before the quit. This PR adds no CI step or job (repo rule: no new CI gates); run it yourself on a GUI host.

Numbers (mac26, 8 workspaces / 32 terminals, 5 browser tabs, 1 agent pane)

Quit (request to process exit) base 7819daf this PR
interactive (Cmd-Q / quit Apple event), live pages 1.91 s 1.71 s
scripted (cmux app quit) 0.13 s 0.12 s
SIGTERM 0.12 s 0.11 s
engine stop wedged on the main thread (debug.quit wedge_engine_stop) 15.04 s 4.04 s (4 s deadline)
Restart to Update, click to old app exit (update-harness, real Sparkle) 85 ms 63 ms
Restart to Update, click to new window 1114 ms 1178 ms
Restart to Update, windows hidden at the click (+24 ms, before Sparkle has the install) at will_relaunch (+22 ms)
Launch (from main(), which comes about 1.5 s after process start) base this PR
first window frame ~0.45 s 0.43-0.49 s
first live terminal frame ~0.75-1.25 s 0.66-0.96 s
longest main-thread stall 0 ms 0 ms
after a daemon version handoff (28 workspaces, 112 terminals) terminal at 14.7 s from process start; daemon.ensure 8.4 s unchanged: follow-up cx-p00l3.1

Samples: sample every 2 s during each quit and launch are in the job artifacts (nx-remote mac26 jobs 1020-200458-a88a5f base, 1020-204732-4fdd44 this PR). Base wedged quit main thread, every sample: semaphore_wait_trap <- QuitDeadline.wedgeMainThread <- closure #6 in QuitCoordinator.complete until the old 15 s deadline.

Keep-quit time budget (4 s from the decision)

  • Quit mark ≤ 0.5 s (QuitDeadline.quitMarkLimit), "Don't ask again" ≤ 0.3 s, then the saves in order: browser records 0.3, windows 0.5, sidebar 0.3, Home's cache 0.4, chat composers 0.4, terminal frames 0.2, agent images 0.2, remote snapshots 0.2, Cloud 0.1 (2.6 s if every step hangs).
  • Late saves before the engines: until 4.0 − 0.2 (reply margin) − 2.0 (engineCloseWait) = 1.8 s after the decision. Then the engine stop, then the wait for every step still running until 3.8 s, then AppKit's reply.
  • Chat composers: the step starts after the quit mark and the four saves before it (in practice tens of ms; 2.3 s if every one hangs). Each page's ask is ≤ 1.0 s (the page's 500 ms draft settle plus 500 ms for a large chip) and the write is ≤ about 0.3 s, so it ends inside the 1.8 s wait when it starts within 0.5 s. A later one runs on beside the engines' close (the asks are async on the main actor, the write is detached) and the last wait covers it until 3.8 s (2.3 + 1.0 + 0.3 = 3.6 s, theComposerAskFitsTheKeepQuitDeadline). Only CefShutdown itself, which blocks the main thread, can delay its last page answers.
  • The trade-off when a save hangs: Chromium's close and its last cookie and storage flush keep their 2 s; a late save gets the time before and after it.

Cloud credential cleanup at quit (trade-off)

On feat, a quit awaited cloud.prepareForTermination() to its end before any save: the Codex broker's stop (SIGTERM, up to 2 s grace) and one revoke per VM it delivered to (each bounded at 3 s, about four or five app-server round trips: isLive, read access.json, read the mode file, remove, restore or remove). With no keep deadline, a slow network delayed the whole quit by that much.

Now Cloud's cleanup starts first and gets all the time the quit has left: the save steps' time, the engine stop, then what is left of the 4 s keep deadline (End quit: the 3 s engine-stop deadline) less 200 ms. On a normal network that is enough for the broker stop and a few revokes. When it is not (a slow network, several VMs, a broker that ignores SIGTERM), the deadline ends the process first, and access.json and the own mode file stay on the VM until the delivered token expires. That is the trade-off of a quit that always ends: the token's expiry is the backstop. plans/cmux-next/OWNERS.md has no row for the Cloud broker (CmuxNextCloud/Broker/); its author (#26170, #26212) is asked for an OK in a PR comment.

Risk tier

risk:high (updater). No default-off setting: both changes fix user-visible regressions (the click-time hide that left cmux invisible for 7 s in the incident, and a wedged quit that took 15 s), which docs/ci/risk-tiers.md ("Ship risky behavior dark") exempts. Automatic installs keep feat's behavior (no hide).

Review of c0756ab (FIX) and its fixes

  • Finding 1 (keep quit could end before the saves): fixed as above. Red 740d5c33d96, fix 0088e20ec76.
  • Finding 2 (an automatic install could stay hidden): fixed as above. Red bbfcfffe854, fix 0088e20ec76.
  • Stale text: this description, the title, restore-journey.py (4 s), the UpdaterService doc of the removed hook.
  • Fleet job ids are in a PR comment.

Review of 0088e20 (FIX) and its fixes

  • Finding 1 (a step past its limit was cut by the normal exit): the quit waits for late steps up to its deadline; End quits wait for late saves before the daemon stops; composer pages are asked within 250 ms. Red c2e92cb8e49, fix 19e931e1183, f95bdd0a0ef (the "Don't ask again" limit moves into QuitSteps), ce9cd46ad51 (Cloud's in-step wait 100 ms, so the limits fit with the reply margin).
  • Finding 2 (Cloud cleanup): the same wait gives Cloud the time left; the trade-off is above.
  • Finding 3: the budget test reads QuitDeadline.quitMarkLimit (the bound quitBegan passes); a test covers installAbandoned on the automatic failure branch.

Review of ce9cd46 (FIX) and its fixes

  • Finding 1 (the 250 ms ask was shorter than the page's 500 ms draft settle): the ask is 1 s again; the page's settle time is one shared value in schemas/agent-pane/wake-state.json; a page that does not answer is logged. Tests: theQuitAsksLongerThanThePageSettlesItsDraft (reads the JSON), theComposerAskFitsTheKeepQuitDeadline.
  • Finding 2 (a fixed 20 ms sleep and polling loops in the quit tests): the tests wait on the limiter's .limit mark and the manual clock's sleeper.
  • Note 3 (a late browser-records save during the engine stop): checked; a closed CEF page returns no navigation list, so its saved session would keep only the shown page. Every late save now lands before the engines stop.
  • Red 49563554e39, fix 654ab7183ff, merge of origin/feat-cmux-next 2cf3babb845 (the branch was 187 commits behind, and the web lint judged feat's own newer files; after the merge it passes).

Review of 6f77b0a (FIX) and its fixes

  • Finding 1 (this PR's own red "Web bundle key inputs"): schemas/agent-pane/* added to BUNDLE_INPUTS in scripts/ci/cmux_next_route.py and schemas/agent-pane/** to on.push.paths of cmux-next-web-bundle-publish.yml. These are two path lines in two existing filters: no new CI gate, job, step or test. bash tests/test_web_bundle_key_inputs.sh: web-bundle-key inputs: ok (it failed before); python3 -m unittest tests.test_cmux_next_route: 111 tests OK.
  • Note 2 (a hung save took the engines' close time): late saves before the engines end at deadline − reply margin − engineCloseWait; a closed page has no session to save. Tests: aHungSaveLeavesTheEnginesTheirCloseWait, aClosedPageHasNoSessionToSave.
  • Nits: aHungCloudStepHoldsNoSave waits on a signal; the QuitSteps doc and this description now say what an End quit does.
  • Routing d49e188e628, red 243c9d3333f, fix c99b21a4ff9.

beforeunload

App quit does not show a page's question, but Chromium's forced close (cmux_shim_close_all, CloseBrowser(true)) still runs each page's beforeunload handler: the test pages' beacons arrive about 0.5 s after close-all. The quit waits for it at most the 2 s close wait. Follow-up cx-p00l3.2 (renderer fast shutdown in the shim).

Changelog

Fixed: Restart to Update never leaves cmux invisible while the installer starts, and a quit that keeps your sessions always ends within 4 seconds.

Observed / Not run

  • Ran (at an earlier head) scripts/cmux-next/quit-launch-bound.py on mac26 (GUI host) with fleet-built apps: base 7819daf + check (red: wedged quit 15.04 s), this PR (green, 2 rounds of interactive, scripted, signal, wedged; session census kept: 9 workspaces, 32 terminals, 5 browser tabs, 1 conversation, 1 page).
  • Ran --handoff --workspaces 28 on base and this PR: shows the daemon adoption delay (not changed here).
  • Ran the update harness (scripts/cmux-next/update-harness/job.sh, real Sparkle update between two signed copies) on base and this PR on cmux-lawrence-2: both relaunched.
  • Gate (gate-run.sh): CmuxNext suites QuitSessionsTests, MainThreadWatchdogTests, MainThreadWatchdogPolicyTests, SilentBackgroundUpdateTests, UpdateCheckFeedbackTests, UpdateReadyCardLayoutTests, RestorePreviousLaunchTests, CrashNoticeTests on Xcode 26.6 (cmux-ci), the Release compile, check-concurrency, godfile check.
  • Not run: the Sparkle failure path itself (a second session asking a live installer to extract again, Sparkle's 7 s timeout). I found it from the unified log and Sparkle 2.9.5 source; I did not reproduce it (follow-up cx-p00l3.3). The fix is the hide timing, which the update harness covers on the success path only.
  • Not run: Xcode 27 compile of the app target (the fleet dev build compiled it on the Linux pool).

🤖 Generated with Claude Code

@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 21, 2026
@cursor

cursor Bot commented Oct 21, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot 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

Copy link
Copy Markdown
Contributor Author

Correctness review (subagent), and resolution

Reviewed head 84f3ee1 (before the fixes below). Current head 03fc2a5.

# Finding Severity Resolution
1 "${advisory[@]}" with set -u fails on macOS bash 3.2 when the array is empty; the CI step would go red on every unloaded host. blocker Fixed: a plain string variable ($advisory).
2 windows step: closeIncognitoWindowsForTermination (one daemon round trip per incognito workspace) ran before recordSaver.flushSaves(), so a slow daemon could push the record flush past the 500 ms limit. should-fix Fixed: records flush first (they never hold incognito windows), then the incognito close, then a second flush.
3 A windows step past its limit could set isTerminating late while saves still ran. should-fix Fixed: WindowManager.stopSavingForTermination() runs right after the step, whatever it did.
4 More quits can now end by _exit, which skips applicationWillTerminate (child process stops). should-fix Accepted, documented: _exit happens only past the 4 s deadline (a hung step); before this PR the same hang never exited or exited by _exit at 15 s. quitBegan writes quitting first, so the next launch is a normal one, not a crash.
5 A shorter close wait (was 3 s) skips CefShutdown (cookie/storage flush) more often. should-fix Changed: close wait 2 s (Chromium gives beforeunload about 1 s), keep deadline 4 s, End deadline 3 s. No cookie flush API exists in the shim; follow-up cx-p00l3.2.
6 The check measured only time, not that the session survived. should-fix Fixed: after each relaunch the check compares the session census (workspaces, tabs by kind) with the one before the first quit and fails on a loss. Green on mac26 for 8 quits incl. 2 wedged.
nit Worst case before the engines stop is about 3.1 s, not 2.6 s. nit Fixed in the QuitDeadline doc.
nit A rollback relaunch also hides the windows now. nit Intended (the quit follows at once); said in the commit.
nit The End path has no deadline before its engines stop. nit Unchanged: endSessionsAndStop has its own 60 s timeout and shows its failure to the user; the PR text says the hard bound is for keep quits.

Checked fine by the reviewer: the keep deadline is armed only after every question is answered; arm keeps the first deadline; QuitStepLimit cannot resume twice; Sparkle calls updaterWillRelaunchApplication for both the user and the automatic driver, and a stage 2 failure still restores the windows; the workflow outputs and needs lists.

The CmuxNextUpdater edits (log level, docs) moved to #22920 so this PR stays risk:normal.

@lawrencecchen
lawrencecchen force-pushed the feat-cmux-next-slowquit branch 2 times, most recently from 11c3df7 to 25f9a34 Compare October 21, 2026 04:34
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review follow-up after the fixes: the step limit moved out of CmuxNextApp (it was 23 lines over the app size cap) into CmuxNextControl as QuitDeadline.step; rebased on feat-cmux-next 9663d4b and wrapped the new agent chat composer save step (#22790) in its own 1 s limit (its pages answer within 1 s each). Godfile check: WindowManager.swift 417 lines (base 407, already over 400) and the WindowManager type +10 lines; the check is warn-only on feat-cmux-next. Head: 25f9a34.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Oct 21, 2026
@lawrencecchen
lawrencecchen force-pushed the feat-cmux-next-slowquit branch from 25f9a34 to a59630b Compare October 21, 2026 04:46
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review a1ec641: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr22919 -f ref=feat-cmux-next-slowquit -f platform=macos -f backend_mode=local

Gallery before/after for a1ec641: 16 states rendered, 0 images differ, 1070 more touched states not rendered. Images are in the run's artifact, not on this public PR; the independent review of this head reads them on its own, so the PR needs no hand-made screenshots of these states.

gh run download 41206346153 -R manaflow-ai/cmux -n gallery-advisory-22919-a1ec641e0bfaf60bd087f9a98651fc274b2b5b9a (then read gallery-review.json)

States
State Theme Status Diff Picked by
agent-pane.chat-width/standard Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.chat-width/standard Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.chat-width/wide Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.chat-width/wide Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.chat-width/wide-blocks Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.chat-width/wide-blocks Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-14 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-14 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-14 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-14 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-tab-14 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-tab-14 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-21 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-21 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-21 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-21 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-tab-21 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-tab-21 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-28 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-chat-28 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-28 Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer/font-size-new-chat-28 Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/short-edge Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/short-edge Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/wide-edge Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/wide-edge Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/no-edge Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-edge/no-edge Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-floats/scrolled-to-end Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-floats/scrolled-to-end Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-floats/composer-grown Apple System Colors Light unchanged 0% imports composerDraft.ts (1 hops)
agent-pane.composer-floats/composer-grown Apple System Colors unchanged 0% imports composerDraft.ts (1 hops)

Pick other states with a Gallery: entry.id, entry.id/variant line in the PR body.

Passes: CI passes on a1ec641e0b.

CI passes on a1ec641e0b (run 41217689368 attempt 1).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way, seen on other PRs that it failed on another pull request's run lately.

web and size checks: green

Default changes

This pull request changes 1 default at a1ec641e0bfa (what everyone gets without configuring anything):

  • tunable-source.CmuxNextApp/Quit/QuitCompletion.swift:run: "f89c92764a68830eab7e9227b679147df46cbe128ece3b46131af9609379ae54" -> (none)

Needs a review. It passes with an independent review receipt on this commit: a comment or review whose first line is Review (<reviewer>): SHIP at a1ec641e0bfaf60bd087f9a98651fc274b2b5b9a, from a member, not a self-review (the newest verdict naming the head decides; a later BLOCK beats an earlier SHIP), or with an approving review on this commit from @austinywang or @teamleaderleo (the call owners in .github/defaults-call-owners.json, never the author). A receipt posted as a review re-runs this check; after a plain comment, re-run it. The needs a call label alone does not pass, and a new push needs a new receipt.

Merge receipt: merged as f76de207f1b3 with these defaults changed, WITHOUT a SHIP receipt or an approving call owner review on its head.

Regenerate the snapshot with python3 scripts/ci/defaults_guard.py snapshot.

Shipped in:

@teamleaderleo

Copy link
Copy Markdown
Collaborator

cmux-next guards / defaults-snapshot is red on 25 of 27 fresh feat-cmux-next PR heads in the last 2:00:00, so it is most likely a base red, not this PR's. The base-reds lane has it; a base merge picks up the fix once it lands.

lawrencecchen and others added 4 commits October 20, 2026 21:58
A tagged app builds a day's session (terminals, Chromium tabs with
beforeunload pages, a WebKit tab, an agent chat), quits it by the
interactive, scripted and signal paths and launches again. It fails when a
quit takes longer than its bound, a launch misses its window or
interactive bound, the launch's main thread stalls past its bound, or an
app quit runs a page's beforeunload. sample records every quit and launch.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A step in cmux-scheme-compile runs scripts/cmux-next/quit-launch-bound.py
on the built app, and the cmux-next quit and launch bound job is its red
check (the perf gate's pattern, so a slow quit never fails the
app-producing job). On a host over the launch load threshold the launch
bounds only warn; the quit bound always holds.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…wedged

The wedged mode arms debug.quit {wedge_engine_stop: true} before the
interactive quit: the quit's engine stop blocks the main thread for good,
and the app must still end within the quit bound by its own deadline. On
the base the deadline is 15 s after the engines begin to stop, so this
mode is red there.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… to Update hides the windows only when Sparkle relaunches

Slow quit and update, 2026-10-20 (nightly 3951982493001, cx-p00l3).

Restart to Update: the click hid every window (transparent, click-through)
before Sparkle took the install. That install never reached the installer:
Sparkle gave up after its 7 s first-message timeout (unified log: windows
hidden 19:28:43.545, back 19:28:50.736, the quit Apple event only after a
second click at 19:29:09.8; the quit itself then took 0.29 s). For 7 s the
app showed nothing. The windows now leave the screen at
updaterWillRelaunchApplication, which Sparkle calls only on the path that
sends its installer to stage 2 and quits the app (a rollback relaunch calls
it too: intended, the quit follows at once).

Quit: a quit that keeps the sessions now ends 4 s after it is decided,
whatever any step does (QuitDeadline.keepQuitLimit, armed at the decision,
after every question was answered). Each save step has its own limit
(QuitDeadline.step: remote snapshots and agent images 300 ms; browser records,
windows and sidebar 500 ms); a windows step past its limit stops saving at
once, and the window records now flush before the incognito close. The
engines' close wait is 2 s (was 3 s; Chromium gives beforeunload about 1 s)
and an End quit ends 3 s after its engines begin to stop (was 15 s). Each
quit step writes a quit.* launch mark. The quit origin and choice log at
notice level, so they stay on disk.

Measured (scripts/cmux-next/quit-launch-bound.py on mac26, 32 terminals,
5 browser tabs, an agent pane): quit with the engine stop wedged 15.04 s
before, 4.04 s after (4 s deadline); other quits unchanged (0.11-2.5 s).
Real Sparkle update (update-harness): click to the old app's exit 85 ms
before, 63 ms after; windows hide at will_relaunch (+22 ms) instead of
before the hand-off to Sparkle (+24 ms).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@lawrencecchen
lawrencecchen force-pushed the feat-cmux-next-slowquit branch from 271aeab to b0315e9 Compare October 21, 2026 05:01
@lawrencecchen lawrencecchen added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Oct 21, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Gate receipt for head b0315e9 (gate-run.sh, all green): CmuxNext suites QuitSessionsTests, MainThreadWatchdogTests, MainThreadWatchdogPolicyTests, SilentBackgroundUpdateTests, UpdateCheckFeedbackTests, UpdateReadyCardLayoutTests, RestorePreviousLaunchTests, CrashNoticeTests (cmux-ci, Xcode 26.6); Release compile (check-release-compile.sh); check-concurrency; check-app-size. Behavior check on mac26 at a59630b (same code, rebased since): every quit and launch within its bounds, wedged quit 4.03 s. Labeled ready-to-land for next-autoland (the workflow file moves on the base faster than a direct gated push).

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Check classification: defaults-snapshot red is a base finding. It reports the settings exports stale against CmuxNextSettings sources (CmuxConfigSnapshot.swift, SettingsSchema*.swift, TrustSettings.swift and others); this PR changes no CmuxNextSettings file (run 39547380903).

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Review (Codex gpt-6-astra xhigh): SHIP at b0315e9. No findings. It conflicts with feat-cmux-next, so it needs a rebase to land; the rebased head needs a fresh review. @lawrencecchen

@teamleaderleo teamleaderleo removed the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 27, 2026
@github-actions github-actions Bot added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 27, 2026
Conflicts resolved:
- QuitCoordinator: kept feat's unsaved registry, Cloud prepareForTermination
  and Home cache flushes, and the PR's step limits and quit marks. Home's
  quit flush in the save steps runs as its own step ("home_cache", 500 ms);
  the engine stop still awaits Home's flush, as on feat. The deadline uses
  feat's reason form, named for both arm points.
- QuitDeadline: feat's reason-based fault line with the PR's limits.
- UpdaterService+App: feat's relaunch-behind, relaunch blockers and
  UpdateWindowHandoff(unsaved:), with the PR's move of the window hide from
  the click to willRelaunch.
- cmux-next.yml: feat's side. The PR's "Quit and launch bound" CI step and
  quit-launch-bound job are dropped: no new CI jobs, steps or gates
  (CLAUDE.md, Lawrence 2026-10-23). scripts/cmux-next/quit-launch-bound.py
  stays, to run by hand or on the fleet.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Conflict resolver (cmux-ad merge order, 2026-10-31): this PR is not pushed. The author must make the changes below.

I merged origin/feat-cmux-next at 8a274c2 into this branch and resolved the 4 conflicts (local merge commit c2773a0, worktree /work/wt/resolve-22919 on the cmux-ad VM; not pushed):

  • QuitCoordinator.swift: kept feat's unsaved registry, cloud.prepareForTermination() and the Home cache flushes, with this PR's step limits and quit marks. Home's quit flush runs as its own step (home_cache, 500 ms). The engine stop still awaits it, as on feat.
  • QuitDeadline.swift: feat's reason fault line with this PR's limits. The coordinator's deadline names both arm points.
  • UpdaterService+App.swift: feat's relaunch-behind, relaunch blockers and UpdateWindowHandoff(unsaved:), with this PR's move of the window hide from the click to willRelaunch.
  • cmux-next.yml: feat's side. The "Quit and launch bound" CI step and the quit-launch-bound job are removed, because CLAUDE.md now forbids new CI jobs, steps and gates (Lawrence 2026-10-23). scripts/cmux-next/quit-launch-bound.py stays; run it by hand or on the fleet.

python3 scripts/cmux-next/prepush-gate.py refuses the push. The refusals come from this PR's own code, measured against ratchets that feat added after this PR opened:

  • global_mutable_statics +1: QuitDeadline+Step.swift:7 @MainActor public static var stepMark. Give the mark sink to step as a parameter, or make the step runner an instance that the coordinator owns.
  • app_lines +57 in CmuxNextApp (QuitCoordinator +33, WindowManager +5, UpdaterService+App +4 by app-size). Move the step code to its domain package, or move the same number of App lines out.

I did not use a skip reason, because these files are in this PR. The ready-to-land label stays as it was. The merge loop does not take this PR while it is DIRTY.

…l; no static mark sink

The quit's step limiter was a static entry point with a static mark sink
(`QuitDeadline.stepMark`, a global mutable static). It is now an instance,
QuitStepLimiter, that QuitCoordinator builds with its launch-mark sink and an
injectable clock. The save steps, their order and their limits (QuitSaveSteps)
and the quit's step sequence (QuitSteps, was App QuitCompletion) move into
CmuxNextControl beside QuitDeadline; QuitCoordinator only fills them from
AppServices. The Restart to Update hide timing is documented on
UpdateWindowHandoff. CmuxNextApp lines: -20 against feat (was +57).

QuitStepLimiterTests (CmuxNextControlTests): a step that ends in time is
marked, a hung step passes its limit and the quit goes on, and the save steps
run in order with the window saves stopped right after the windows step.
…eam iterator cannot be advanced on the main actor

The fleet compile of 2cf3bab (job 0d9400b1ab624f3ed7e3e1ca) refused Marks.nextLimit:
'cannot call mutating async function next() on actor-isolated property iterator'.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fixes for the third FIX review at ce9cd46 (#22919 (comment)). Head 6f77b0a.

Commits: red 49563554e39; fix 654ab7183ff; merge of origin/feat-cmux-next 2cf3babb845; 6f77b0ac42f (test helper: the limit-mark waiter resumes a continuation; the fleet compile at 2cf3bab, job 0d9400b1ab624f3ed7e3e1ca, refused the AsyncStream iterator on the main actor, so no test ran there).

Focused command: scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext <suites> (fleet cmux-ci run --class light).

  • Red: 05ae1c4e7c5 (branch evidence/pr22919-red3) = red 49563554e39 plus only the test-helper compile fix, so the red runs. Step 0deadef41619fc776d8d7455 on cmuxs-Mac-mini:
    • aStepPastItsLimitStillWritesBeforeTheEnginesStop failed for agent_composers and browser_records: order ["engines_stopped", "written", "replied"], so the late save ran after the engines stopped.
    • aStepThatNeverEndsHoldsTheQuitOnlyForTheTimeLeft failed: the engines stopped while a save still ran.
    • theQuitAsksLongerThanThePageSettlesItsDraft failed: ask 0.25 s, but the page settles for 0.5 s plus 0.5 s for a large chip.
    • anEndQuitWaitsForALateSaveBeforeItEndsTheSessions passed: the End-quit wait was already in the code from 19e931e. This round only removes its real-time sleep.
  • Green at 6f77b0a: step 29c5cd86c841dceff71ac1b8 on aws-m4pro-7. All 8 suites passed: QuitStepLimiterTests, ForcedQuitExitTests, AgentPaneRelaunchWakeTests, UpdateRelaunchHandoffTests, UpdateInstallStatusTests, UpdateRelaunchFocusTests, QuitSessionsTests, RestorePreviousLaunchTests.
  • Compile at 6f77b0a: job 647714d876d0a337eb20576f passed (CmuxNext 297 s, app 48 s, cmux-tui 42 s).
  • scripts/web-lint.sh: passed after the merge of feat. Before the merge, it failed only on four feat files this PR does not touch (the branch was 187 commits behind).
  • prepush-gate.py: no new violations; app_lines falls; no exception.

Note 3 (browser records): a closed CEF page returns no navigation list (CEFTab.nativeNavigationList needs a live browser), so a late browser-records save after the engine stop would keep only the shown page. Every late save now lands before the engines stop; only Cloud's cleanup may run on until AppKit's reply.

Not run: the quit-launch-bound bench and the update harness at this head (GUI host); a live quit with a chat page whose draft is still settling.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-review-subagent-4): FIX at 6f77b0a

Independent review of the full diff from merge-base a718e9f to 6f77b0a. I also read the repair ce9cd46ad517..654ab7183ff and 2cf3babb845..6f77b0ac42f, and the three earlier reviews. I read the code and the CI logs only. I did not build or run anything.

Findings

1. The PR causes its own red check: "Web bundle key inputs" (must fix)

  • webviews/src/agent-session/acpmux/composerDraft.ts:3 now imports schemas/agent-pane/wake-state.json, a file outside webviews/. The repository requires every such import to be routed, so that a change to it rebuilds and publishes the web bundle.
  • tests/test_web_bundle_key_inputs.sh fails on this head (cmux-next checks job 124534992241, step "Web bundle key inputs"):
    FAIL: imported web bundle inputs are not routed:
      schemas/agent-pane/wake-state.json: not in cmux-next-web-bundle-publish.yml on.push.paths
      schemas/agent-pane/wake-state.json: not in scripts/ci/cmux_next_route.py BUNDLE_INPUTS
    
  • Effect: a commit that changes only draftSettleMs does not start the bundle publish and does not select the bundle tier. Mac builds, the CI cache and the HQ Linux pool can then ship an agent pane with the old settle time, while the host test reads the new value (the cx-wtsf3 class of bug). The "one shared value" then stops being one value.
  • This red is from this PR, not from the base: the only path the check names is the new file. CLAUDE.md says a red the PR causes keeps it out.
  • Fix: add "schemas/agent-pane/*" to BUNDLE_INPUTS (scripts/ci/cmux_next_route.py:233-234, next to "schemas/sync-motion/*"), and add - "schemas/agent-pane/**" to the on.push.paths filter of .github/workflows/cmux-next-web-bundle-publish.yml (next to line 40). cmux-next.yml already filters schemas/** (lines 73, 353). This adds a path to existing filters, not a new CI gate, job or step.
  • The bundle itself reads the JSON correctly: the path ../../../../schemas/... resolves to the repository root from webviews/src/agent-session/acpmux/; the file is under 8 KB, so jsonParsePlugin.mjs returns undefined and esbuild's default JSON loader inlines it; schemas/settings/settings-schema.json (src/pages/settings/schema.ts:3) and schemas/sync-motion/streams.json are the same pattern. Only the routing is missing.

2. Note (no change required for SHIP): a late save now uses the engine stop's time

  • QuitSteps.swift:85 waits for late saves for timeLeft(), which is the deadline less only the 200 ms reply margin. If a save is still running at that point (a daemon that does not answer the windows or browser-records step), the engine stop starts with about 200 ms left. cef.shutdown(timeout: engineCloseWait) (QuitCoordinator.swift:235) then cannot finish its close (the PR body says the beforeunload beacons alone take about 0.5 s), so CefShutdown and Chromium's last cookie and storage flush do not run before _exit.
  • At ce9cd46 the engines stopped before this wait. This is a trade-off between two kinds of the person's data, and it occurs only when a save hangs. Write it in the PR body next to the Cloud trade-off, or keep engineCloseWait free (for example timeLeft(margin: replyMargin + engineCloseWait) for the wait before the engines and the full margin after).

3. Nits

  • The PR body says the tests have "no real-time sleeps". QuitStepLimiterTests.swift:80 (aHungCloudStepHoldsNoSave) still polls with Task.sleep(for: .milliseconds(2)) up to 500 times. It is bounded and can only fail on a very slow host, not pass wrongly. A Marks-style signal on remote_snapshots would remove it.
  • For an End quit, the deadline is not armed before stopBrowserEngines, so timeLeft() ?? .zero at QuitSteps.swift:85 is zero. A save still late after the 2 s End wait (:77) therefore runs on after the engines stop. The doc at QuitSteps.swift:19-21 and the PR body ("Every late save also lands before the browser engines stop") say otherwise. This has no practical effect (the daemon is already stopped, and End Everything deletes the workspaces), but correct the text.

Checked and fine

  • 4 s keep bound, worst case (t from quit.decided; the deadline is armed first, QuitCoordinator.swift:176, on its own thread):
    • quit mark ≤ 0.5 s (markQuitting(timeout: quitMarkLimit) returns at its timeout) → 0.5 s
    • "Don't ask again" ≤ 0.3 s (rememberLimit, its write past the limit is a late step) → 0.8 s
    • save steps 0.3 + 0.5 + 0.3 + 0.4 + 0.4 + 0.2 + 0.2 + 0.2 = 2.5 s → 3.3 s; Cloud 0.1 s → 3.4 s
    • wait for late saves ≤ 4.0 − 3.4 − 0.2 = 0.4 s → 3.8 s
    • Home's second flush, the 2 s close wait and CefShutdown: the deadline thread ends the process at 4.0 s whatever they do, also with the main thread blocked.
    • The composer step: it starts by 0.5 + 0.3 + 0.3 + 0.5 + 0.3 + 0.4 = 2.3 s; its 1 s ask ends by 3.3 s; its write has until 3.8 s (0.5 s). theComposerAskFitsTheKeepQuitDeadline checks 2.3 + 1.0 + 0.3 + 0.2 = 3.8 ≤ 4.0.
  • The wait cannot hang the quit. finishOverruns races the late tasks against a DemandTimer on the limiter's clock; a limit of zero returns at once; a timeout returns and cancels only the awaiting task, not the steps. timeLeft is clamped at zero, and nil (End quit before the engines) becomes zero. No main-thread wait.
  • Host/page budget (earlier finding 1). askLimit is 1 s (AgentPaneRelaunchWake.swift:150), the default of collect. The page waits DRAFT_SETTLE_MS = draftSettleMs (500) and then answers (App.tsx pane.wakeState). The host test reads the same JSON key through #filePath (six levels up is the repository root) as an Int. A page that does not answer is logged, and only on a real timeout (answer == nil, the task not cancelled).
  • Tests. The new QuitStepLimiterTests wait on the limiter's .limit mark and on ManualClock.waitForSleeper, with no fixed sleep. aStepPastItsLimitStillWritesBeforeTheEnginesStop and aStepThatNeverEndsHoldsTheQuitOnlyForTheTimeLeft fail with ce9cd46's order (engines before the wait), and theQuitAsksLongerThanThePageSettlesItsDraft fails at 250 ms, as the red run 0deadef41619fc776d8d7455 shows. The green run is 29c5cd86c841dceff71ac1b8 at this head.
  • Earlier findings did not come back. Cloud starts first and holds no save; the step limits fit the deadline; late steps are awaited; the composer ask outlasts the page; the late browser-records save in a keep quit lands before the engine stop; installAbandoned is called on the automatic failure and on a declined relaunch behind.
  • Only a click hides the windows. hidesForRelaunch runs only when !automaticInstallInFlight (UpdaterService+ActionDelegate.swift:22). Rollback calls only willRelaunch. The relaunch behind and a held automatic install that Sparkle relaunches keep automaticInstallInFlight true, so they do not hide. A clicked install that fails (.note with wasInstalling), the 180 s stall give-up, the automatic failure and quitForInstallDeclined all call installAbandoned; restore() does nothing when nothing is hidden; hide() does nothing with unsaved documents, which is the only refusal a keep relaunch can get.
  • feat behavior. relaunchBehind, relaunchBlockers and UpdateWindowHandoff(unsaved:) are unchanged (UpdaterService+App.swift).
  • Merge. The diff of 2cf3babb845^2..6f77b0ac42f is the same as the PR diff from the merge-base, so the merge lost no feat change. The 47 newer feat commits touch only WindowManager.swift (a new backgroundClaims property) and App.tsx (not pane.wakeState), with no overlap.
  • No new static mutable state, singleton or main-thread block. New statics are let; the limiter is an instance that QuitCoordinator owns; QuitDeadline.firesAt is an instance Mutex.
  • Other reds on this head: defaults guard is the deleted QuitCompletion.swift:run tunable fingerprint and needs only a SHIP receipt; risk evidence waits for that receipt; the namespace-type error is in RightColumnPlacement.swift (not in this PR); release-notary fails on a missing xcodebuild and a refused App token (infrastructure). cmux-next swift test and fast tier are pending.

After finding 1 is fixed (two path lines), I expect SHIP. Findings 2 and 3 are optional.

…path lines; no new gate, job or step)

Review of #22919 at 6f77b0a, finding 1: composerDraft.ts imports
schemas/agent-pane/wake-state.json, so a change to it must rebuild and publish the web
bundle. This adds "schemas/agent-pane/*" to the existing BUNDLE_INPUTS list in
scripts/ci/cmux_next_route.py and "schemas/agent-pane/**" to the existing on.push.paths
filter of cmux-next-web-bundle-publish.yml, next to schemas/sync-motion. It adds a path to
two existing filters only: no new CI gate, job, step or test.
tests/test_web_bundle_key_inputs.sh: "web-bundle-key inputs: ok" (it failed before).
… a closed page has no session to save (red)

Review of #22919 at 6f77b0a, note 2: the wait for late saves before the engine stop
ran to 200 ms before the deadline, so a hung save left Chromium no time to close its pages
(CefShutdown's last cookie and storage flush lost). aHungSaveLeavesTheEnginesTheirCloseWait
fails here: the engines have not stopped once only engineCloseWait is left.
Reserving that time lets a late browser-records save run on beside the engine close, where
a closed page's session keeps only the shown page; aClosedPageHasNoSessionToSave fails
here (BrowserRestoredSession answers for a closed page).
Nit: aHungCloudStepHoldsNoSave waits on a signal, not a 2 ms poll.
…ve; a closed page has no session to save

Review of #22919 at 6f77b0a, note 2 and nits.
QuitSteps waits for late saves before the engine stop only for what the deadline leaves
less QuitDeadline.engineCloseWait (and the reply margin), so Chromium still closes its
pages and CefShutdown flushes cookies and storage. A save still late then runs on beside
the engine stop, and the wait before AppKit's reply covers it. BrowserRestoredSession
answers no session for a closed page (as its other entry points already do), so a late
browser-records save writes nothing over the good record the daemon holds. The QuitSteps
doc now says what an End quit does: its deadline is armed only as the engines stop, so a
save still late after endQuitSaveWait runs on beside the engine stop.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fixes for the fourth FIX review at 6f77b0a (#22919 (comment)). Head c99b21a.

Commits:

  • d49e188e628 routing: adds schemas/agent-pane/* to BUNDLE_INPUTS and schemas/agent-pane/** to the publish workflow's on.push.paths. These are two path lines in two existing filters: no new gate, job, step or test.
  • 243c9d3333f red tests.
  • c99b21a4ff9 fix.

Static checks on the agent VM (no build):

  • bash tests/test_web_bundle_key_inputs.sh: web-bundle-key inputs: ok. Before the routing commit it printed FAIL: imported web bundle inputs are not routed for both files.
  • python3 -m unittest tests.test_cmux_next_route: 111 tests, OK.
  • scripts/web-lint.sh: passed.
  • prepush-gate.py: no new violations; app_lines falls; no exception.

Fleet (scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext <suites>):

  • Red at 243c9d3 (--red): step 16bf29fb44a5f7891ee5fd8e on aws-m4pro-6.
    • aHungSaveLeavesTheEnginesTheirCloseWait failed: the engines had not stopped with only engineCloseWait left.
    • aClosedPageHasNoSessionToSave failed: a closed page still gave a session, and its scroll was measured.
  • Green at c99b21a: step fe35d25fa53ba6bc15af24c0 on aws-m4pro-1, all 9 suites passed: QuitStepLimiterTests, ForcedQuitExitTests, AgentPaneRelaunchWakeTests, UpdateRelaunchHandoffTests, UpdateInstallStatusTests, UpdateRelaunchFocusTests, QuitSessionsTests, RestorePreviousLaunchTests, BrowserScrollReportTests.
  • Compile at c99b21a (recipe 2, --app): job a80168bdac2c1fec39afb409 passed (CmuxNext 200 s, app 80 s, cmux-tui 25 s).

The time budget is in the PR body ("Keep-quit time budget"). Late saves before the engines get until 1.8 s after the decision (4.0 − 0.2 − 2.0). A chat-composer step that starts within 0.5 s ends inside that wait. A later one runs on beside the engine close, and the last wait covers it until 3.8 s.

Not run: the quit-launch-bound bench and the update harness at this head, because they need a GUI host.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-review-subagent-5): FIX at c99b21a

Independent review of the full diff from merge-base a718e9f to c99b21a, of the repair 6f77b0ac42f1..c99b21a4ff9f, and of the four earlier reviews. I read the code and the CI logs. I ran bash tests/test_web_bundle_key_inputs.sh (ok), python3 -m unittest tests.test_cmux_next_route (111 tests, OK) and web-bundle-key.py . --inputs (it lists schemas/agent-pane/wake-state.json). I did not build or run Swift.

Findings

1. The closed-page guard is checked only before the scroll measure, so a measure in flight when the engine closes the page still writes the shown-page-only session (must fix)

  • The repair shortens the wait before the engines to timeLeft − engineCloseWait (QuitSteps.swift:91-92), so a late browser_records save now runs beside the engine stop from 1.8 s after the decision (at 6f77b0a: from 3.8 s). The repair relies on BrowserRestoredSession.savedSession returning nil for a closed page to make that overlap harmless.
  • savedSession checks !tab.isClosed once, at entry (BrowserRestoredSession.swift:117). Then it awaits tab.currentScrollY() (:123). After the await it does not check again, and builds the session from tab.nativeNavigationList() (:129).
  • At quit, each history writer's flushNow starts with this measure (BrowserHistoryWriter.swift:84, 400 ms budget). The measure is a JS evaluation in the page. When Chromium closes that browser during cef.shutdown:
    • CEFRuntime+Events.swift:236 calls browserDidClose. It sets browserID = nil and isClosed = true (CEFTab.swift:302, :308).
    • Then :243 failEvaluations makes the pending evaluation fail, so currentScrollY() returns nil at once, inside the 400 ms budget.
    • savedSession continues. nativeNavigationList() needs browserID (CEFTab+BackForwardList.swift:19), so it is nil. session(around: nil, shown:) (BrowserRestoredHistory.swift:84) keeps only the saved entries of the last launch plus the shown page. Every back/forward entry of this run is lost.
    • flushNow writes that session (BrowserHistoryWriter.swift:85-86). In a keep quit the daemon is still alive, so the session replaces the good record. A flush that starts after close-all is issued but before its own tab's browser closes (the PR body measures about 0.5 s for close-all) has the same problem.
  • So the repair's own claim ("a late flush writes nothing and the daemon's record stays") does not hold in the scenario it is for. The repair also makes that scenario 2 s more likely.
  • aClosedPageHasNoSessionToSave (BrowserScrollReportTests.swift:45) closes the host before the call only. It does not cover a close during the measure.
  • Fix: check !tab.isClosed again after the currentScrollY() await, before :127 and :129. Add a test whose host's currentScrollY() sets closed = true and returns nil, and expects savedSession(measuringScroll: true) == nil. That test fails at this head.

Notes (do not block)

  • "Written after each navigation" (PR body, BrowserRestoredSession doc) is debounced by writeBackDelay (500 ms, BrowserTabService.swift:89). A navigation inside that window before a slow quit relies on the quit flush. When that flush is skipped (closed page), the record writer still moves the URL, and trackHistory then restores no history (the record URL no longer matches the history's page). That is about the same loss as the degraded write, so it is not a regression. Only the text claims too much.
  • Keep-quit worst case, t from quit.decided (deadline armed first, QuitCoordinator.swift:165):
    • quit mark 0.5 s, then "Don't ask again" 0.3 s → 0.8 s; save steps 2.5 s plus Cloud 0.1 s → 3.4 s.
    • Wait before the engines: max(0, (4.0 − 3.4 − 0.2) − 2.0) = 0, so no wait. The engines start at 3.4 s; the deadline thread ends the process at 4.0 s whatever CefShutdown does.
    • A typical quit with one hung save: the wait ends at 4.0 − 0.2 − 2.0 = 1.8 s; the close wait ends by 3.8 s; CefShutdown has the rest. With the measured close (about 0.5 s), CefShutdown starts near 2.3 s and has about 1.7 s.
    • In an End quit, timeLeft is nil before the engines, so beforeEngines is 0 after the 2 s End wait. This matches the corrected doc at QuitSteps.swift:19-29.

Checked and fine

  • Earlier findings did not come back. Cloud starts first and holds no save; the step limits fit the deadline; late steps are awaited (End: before the sessions end; keep: before the engines, then before the reply); the composer ask is 1 s and outlasts the page's 500 ms settle, from one shared JSON value; a page that does not answer is logged; installAbandoned is called on the automatic failure and on a declined relaunch behind; the tests use signals, not sleeps (Reached replaces the last polling loop).
  • Routing (finding 1 of the fourth review). schemas/agent-pane/* in BUNDLE_INPUTS matches with fnmatch (cmux_next_route.py:1055). schemas/agent-pane/** in on.push.paths is the same form as schemas/sync-motion/**. The bundle key hashes the JSON. These are two path lines in two filters that already exist: no new CI gate, job, step or test.
  • isClosed cannot read true for an open tab. CEFTab.isClosed is set only in close() (CEFTab.swift:529) and in browserDidClose (:308), and WebKitTab.isClosed only in its close(). Each app path that closes a page (release, swapPage, replacePage, reroute, retryChromium, rebuildForAgent) calls untrack in the same main-actor turn, which removes the writers. browserDidClose without the quit closes the tab (emit(.close)). BrowserHistoryWriter is the only caller of savedSession; it treats nil as "write nothing" (:85-88, :112). So a normal save never loses a session to the new guard.
  • New tests. aHungSaveLeavesTheEnginesTheirCloseWait is deterministic on the ManualClock: it advances exactly 3 s − engineCloseWait, and the next sleeper is the final wait's timer, which exists only after engines_stopped. With the old within: timeLeft() the 3 s timer does not fire, so it fails (red step 16bf29fb44a5f7891ee5fd8e). aClosedPageHasNoSessionToSave fails without the guard (measured scroll, non-nil session).
  • Only a click hides the windows. hidesForRelaunch runs only when !automaticInstallInFlight (UpdaterService+ActionDelegate.swift:22). A clicked failure, the 180 s stall give-up, the automatic failure (UpdaterService+Flow.swift:214) and quitForInstallDeclined (UpdaterService+IdleInstall.swift:75) call installAbandoned. hide() does nothing with unsaved documents, which is the only refusal a keep relaunch can get. The repair did not change the updater.
  • feat's updater behavior. relaunchBehind, relaunchBlockers and UpdateWindowHandoff(unsaved:) are unchanged (UpdaterService+App.swift).
  • Chat pages are WKWebView, so cef.shutdown does not close them. A composer step past 1.8 s still gets its answers during the async close wait. Only CefShutdown's main-thread block can delay them, as the PR body says.
  • No new static mutable state, singleton or main-thread block. The repair adds one source line of logic and one let. Reached and TimeLeft are test-only instances.
  • Merge. The branch is 66 commits behind feat. The only overlapping files are cmux_next_route.py (another hunk) and WindowManager.swift (other lines); GitHub says MERGEABLE.

CI at c99b21a

  • defaults guard: needs a SHIP receipt for the deleted QuitCompletion.swift:run tunable fingerprint. It is not a code defect.
  • risk evidence: holds risk:high until a SHIP receipt.
  • nightly-guards / workflow-guard-tests / release-notary and its wrapper Guard status: tests/test_ci_signing_owned_mac.py test_every_listed_hook_is_one_glaeda_released_with_the_check (a glaeda release pin not in SIGNING_HOOKS), plus refused App tokens (401/403/429). This PR does not touch signing or glaeda. This is a base finding: the same failure was in the Blacksmith summary at an earlier head.
  • Green: cmux-next checks (with "Web bundle key inputs"), cmux-next app compile (build fleet), cmux-next fleet Release compile, cmux-next/compile, merge freeze, namespace autofix.
  • Pending: cmux-next swift test, fast tier, daemon tests, web / react-apps-check (1-4).

After finding 1 is fixed (one more isClosed check after the await, and its test), I expect SHIP.

…ion to save (red)

Review of #22919 at c99b21a, finding 1: savedSession checked isClosed only before it
awaited currentScrollY(). A quit's engine stop can close the page during that await; the
save then read no back/forward list and wrote a shown-page-only session over the good record.
… the page is still open

The engine can close the page during the await; reading the navigation list after it gave a
session with only the shown page, which a quit's late save wrote over the good record.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-review-subagent-6): SHIP at a1ec641

Independent review of the full diff from merge-base a718e9f to a1ec641, of the repair c99b21a4ff9f..a1ec641e0bfa (two files), and of the five earlier reviews (6541202569, 6542031209, 6543025948, 6543771041, 6544057512). I read the code. I ran bash tests/test_web_bundle_key_inputs.sh (web-bundle-key inputs: ok). I did not build or run Swift.

Findings

None that block.

The repair (finding 1 of review 5)

  • BrowserRestoredSession.swift:129 now checks !tab.isClosed after the currentScrollY() await (:123) and before it reads tab.state.url and tab.nativeNavigationList() (:130-131).
  • The order in the engine makes this check sufficient. CEFRuntime+Events.swift:236 calls browserDidClose, which sets isClosed = true (CEFTab.swift:308). Only after that, :243 failEvaluations makes the pending scroll question fail. So when the await resumes because the page closed, isClosed is already true, and savedSession returns nil.
  • flushNow then calls savedSession(measuringScroll: false) (BrowserHistoryWriter.swift:87). It checks isClosed at entry and has no await before it reads the list, so it also returns nil, and nothing is written over the good record. The same applies when the 400 ms measure budget runs out first, and to a debounced pageDidChange write (:112) that starts after the close.
  • Other awaits in savedSession: there are none. The currentScrollY() await is the only suspension point. After it, the code reads only the live tab state (tab.state.url, title), which is correct for a page that navigated during the await. The knownScrollY write at :125 is still fenced by the URL compare. The tab local is a strong host binding, so it cannot become nil during the await.
  • Test aPageClosedWhileItIsMeasuredHasNoSessionToSave (BrowserScrollReportTests.swift:61): the host sets closed = true inside currentScrollY() and returns nil. At 82e8c11 the entry guard passes, the shown-page-only session (session(around: nil, shown:)) is returned, and the test fails. At a1ec641 it returns nil. The test uses no clock or sleep, so the result does not change between runs. It also asserts measured == 1, so it cannot pass because the measure did not run.

Earlier findings did not come back

The only changes after c99b21a are the two files above. Checked again on this head:

  • Keep-quit bound: the deadline is armed at quit.decided (QuitCoordinator.swift:165) on its own thread, and _exit ends the process at 4 s whatever the main thread does. Worst case: quit mark 0.5 + remember 0.3 + steps 2.5 + Cloud 0.1 = 3.4 s; the wait before the engines is max(0, timeLeft - engineCloseWait) (QuitSteps.swift:91-92); the last wait ends at deadline less 200 ms.
  • Cloud starts first and holds no save (QuitSaveSteps.swift:205). Late steps are awaited (End: up to 2 s before the sessions end, without Cloud; keep: before the engines, then before the reply). finishOverruns is an async race on the limiter clock and never blocks the main thread.
  • Composer ask is 1 s and is longer than the page's 500 ms settle, from one shared value (schemas/agent-pane/wake-state.json). The value is routed in cmux_next_route.py:234 and cmux-next-web-bundle-publish.yml:41. A page that does not answer is logged.
  • Updater: hidesForRelaunch runs only when !automaticInstallInFlight (UpdaterService+ActionDelegate.swift:22). A clicked failure, the 180 s stall give-up, the automatic failure (UpdaterService+Flow.swift:214) and quitForInstallDeclined (UpdaterService+IdleInstall.swift:75) call installAbandoned. hide() does nothing with unsaved documents. feat's relaunchBehind, relaunchBlockers and UpdateWindowHandoff(unsaved:) are unchanged.
  • No new static mutable state or singleton (the limiter is an instance that QuitCoordinator owns; new statics are let). No main-thread wait (the semaphore waits are on the deadline thread and in the DEBUG wedge seam). No new CI gate, job or step.

Notes (do not block)

  • Evidence for this round: the commits do not yet give the fleet red result at 82e8c11 or the green result at a1ec641 for BrowserScrollReportTests. The compile checks (cmux-next/compile, cmux-next app compile (build fleet), cmux-next fleet Release compile) and cmux-next swift test are pending at this head. The risk:high tier needs a green compile together with this receipt. Add the step IDs to the PR when the jobs end.
  • A related race outside savedSession: in BrowserHistoryWriter.write (BrowserHistoryWriter.swift:125), the private-field probe is an await. If the probe answers false and the engine closes the page before the task resumes, navigationList() has no native list, the engine-state branch is skipped, and the write sends the correct entries without engineState (it replaces the recorded one). The entries stay correct; only the Chromium page state (form values, native history) is lost, and only in a narrow window of a keep quit where a late browser-records save overlaps the engine close. If you want to close it, check isClosed again after the probe and keep recorded?.engineState when the page closed. A follow-up is enough.
  • Remaining CI reds at this head: defaults guard (the deleted QuitCompletion.swift:run tunable fingerprint; it needs this receipt) and risk evidence (it waits for this receipt). Neither is a code defect.

@cmux-bot cmux-bot Bot removed the risk:high Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
@github-actions github-actions Bot added the risk:high Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fleet evidence for a1ec641 (review 5, finding 1: a page closed during its scroll measure):

  • Red at 82e8c11 (--red, step 853a4da2de17d838abded1dc, aws-m4pro-7): aPageClosedWhileItIsMeasuredHasNoSessionToSave fails on its assertion (BrowserScrollReportTests.swift:66, savedSession(measuringScroll: true) == nil); the other 6 tests pass. No crash.
  • Green at a1ec641 (step 06de9b1fdde6c934de13a86c, aws-m4pro-2): all 9 suites pass (BrowserScrollReport, QuitStepLimiter, ForcedQuitExit, AgentPaneRelaunchWake, UpdateRelaunchHandoff, UpdateInstallStatus, UpdateRelaunchFocus, QuitSessions, RestorePreviousLaunch).
  • Compile-check at a1ec641 (recipe 2, --app, job 16e5ffd872b393e024d8c5f6): CmuxNext, app and cmux-tui pass.
  • prepush-gate.py: no new violations.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Oct 31, 2026
lawrencecchen pushed a commit that referenced this pull request Oct 31, 2026
…t to Update never leaves cmux invisible (#22919)

merge-batch candidate: the squash merge of #22919 at a1ec641
@cmux-bot cmux-bot Bot removed the stack-blocker label Oct 31, 2026
@lawrencecchen
lawrencecchen merged commit f76de20 into feat-cmux-next Oct 31, 2026
405 of 418 checks passed
@lawrencecchen
lawrencecchen deleted the feat-cmux-next-slowquit branch October 31, 2026 16:54
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…se-quit-flush

Port of #28275 onto feat (lead decision 05:20Z), keeping feat's state-file and
quit work and adding only what this PR adds beyond it:
- AgentUnsentDrafts: feat's #28204 undelivered drafts and unreadable-file
  set-aside, on this PR's per-app AgentPaneStateFiles (no static registry).
- QuitCoordinator: feat's #22919 QuitStepLimiter, time-left reply margin,
  launch marks and engineCloseWait, plus this PR's drains of the state files
  around the engine close, the testable quitSteps() (was steps(), which now
  names feat's limiter) and closeEngines seam, and the deadline end that records
  cut-off files.
- AppDelegate/AppServices: feat's openedURLs, windowRecord, peer approver and
  injected key window; this PR's secondSignalExit (no blocking flush in prepare)
  and the state-files owner and loss record.
- Paired phones: feat's #25437 MobileHostPairedStore now owns the directory's
  one MobileHostPairedDevicesFile (configure(directory:lastQuitCutOff:
  cutOffSettled:)); the iPhone host takes it from the store for the quit watch,
  so the shared-list transactions and this PR's cut-off accounting use one file
  object and one queue (an instance enqueue that counts as saving), not two.

This branch was previously deployed

1 inactive deployment
ci-routing-agent — a1ec641e Deployed Oct 31, 2026 by lawrencecchen via cmux-tui tree preflight #90047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:high Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants