Repository navigation
fix(quit): state files have one owner each and no quit blocks the main thread on them - #28275
Conversation
… the agent panes' state files The forced quit (ForcedQuitExit, a second SIGTERM/SIGINT/SIGHUP while a quit runs) flushed the agent panes' state files in `prepare`, under allowBlocking(.terminate), before its 160 ms deadline was armed: with a save queued behind a busy disk the exit waited up to 2 s on the main thread, and its own deadline did not bound that wait. This commit extracts the exit's steps into AppDelegate.secondSignalExit (same steps, injectable deadline and end) and adds QuitStateFilesTests.theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles. It is red on this commit: the deadline is armed about 2 s after the start, and the exit ends before the queued draft is on disk. The fix is the next commit. Not run here (no Swift build on this VM): the fleet runs it.
…n thread on them Rise audit (arch-ratchet tip rises with no reviewed exception) of cmux#27749 and #25535: - AgentPaneStateFile's process registry (`static let files = Mutex<...>`) becomes the AgentPaneStateFiles instance that AppServices owns and passes to AgentUnsentDrafts and AgentPaneScrollAnchors (composition root, PATTERNS.md). Same one-writer-per-path rule. - MobileHostPairedDevicesFile's `static let queue` and `static let inForce` become members of one file object that IPhoneHostService owns per state directory (it built a new value per use before). This run's record of the list in force is now per file object; a relaunch is a new object. - The three allowBlocking(.terminate) flushes are gone: - ForcedQuitExit `prepare` (AppDelegate): the flush ran before the 160 ms deadline was armed, so a second quit signal could wait 2 s. The exit now arms the deadline first and awaits the state files beside the Home cache's write in `save`. - applicationWillTerminate (AppDelegate) and IPhoneHostService.stop(): up to 2 s each, 4 s in all. The quit's `.terminateLater` phase (QuitCoordinator) now awaits AppServices.stateFilesDrained() in prepareWindows and again in stopBrowserEngines, next to home.flushCacheForQuit(): one 2 s limit for all the files, holding no thread. - QueueDrain (CmuxNextWakeups): waits for serial queues with one continuation and one DemandTimer deadline, so no thread blocks and a quit with no backlog costs no timer fire. - SignalRelay moves from CmuxNextApp/Quit to CmuxNextControl/Diagnostics (beside ForcedQuitExit and QuitDeadline), so CmuxNextApp's lines do not rise. Ratchet (arch_counts --delta): global_mutable_statics -3, blocking_allow_escapes -3, app_lines -31. Regression: QuitStateFilesTests.theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles is red on the previous commit and should be green here; theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread covers the normal quit's wait. Not run here (no Swift build on this VM): the fleet runs them. Owners told: Performance Lead (Wakeup primitives, statics), PL-threads (the terminate allowance).
Bugbot needs on-demand usage enabledBugbot 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. |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
web and size checks: green Shipped in:
|
|
Found 13 test failures on Blacksmith runners: Failures
...and 3 more test failures. View all on Blacksmith: run 41278346468, run 41278346331
|
|
Review (independent review subagent, arch-rise): SHIP at 1869061 What I checked. I did not edit, build or run Swift. HEAD, the worktree and the PR headRefOid are all 1869061. Job c3a700cf compiled this ref (CmuxNext, app, cmux-tui), and its log shows that QuitStateFilesTests.swift, MobileHostPairingTests.swift and the CmuxNextAgentPaneTests module compiled.
Two reds are caused by this PR. Each one needs only a description change, so this receipt stays valid (the head does not change):
Notes (they do not block):
|
…tor members `steps()` builds the QuitSteps that `complete` runs, and `stopEngines` is the engine stop (the deadline, then CefShutdown) that stopBrowserEngines ends with. Same steps in the same order; a test can now run a step through the coordinator with its own engine stop. No behavior change.
…efore it goes on QuitStepsStateFilesTests runs the coordinator's own steps with a new chat's draft save queued behind a busy disk (a held file queue, released after 1 s): - theSaveStepEndsWithTheQueuedDraftOnDisk: prepareWindows returns with the draft on disk. - theEnginesStopAfterTheQueuedDraftIsOnDisk: stopBrowserEngines reaches the engine stop with the draft on disk. Each fails when its step's `await services.stateFilesDrained()` is removed (the red-proof ref). Not run here (no Swift build on this VM): the fleet runs them.
…FirstOutcome One path for "first of an event and a DemandTimer limit wins" (FirstOutcome): QueueDrain keeps only the count of queues still to answer; its own continuation and countdown class are gone.
|
Review (independent review subagent, arch-rise): SHIP at 07f1b95 Scope: the three commits after the earlier SHIP at 1869061 (b976b02, 25a49ee, 07f1b95). The earlier findings stand for the code that review read.
|
`AgentPaneStateFiles(drainBegan:)` is told each time `drained(within:)` begins, before it waits; AppServices takes the registry through its init (default: a plain one). A test holds a file's queue and releases it only there, so only a real drain lets a quit step go on: no wall-clock hold decides a test (lead rule R5). `stateFilesDrained` returns whether every file's saves ran in time.
…lock decides the quit tests The quit tests held a file's queue and let it go on a timer (1 s), or measured wall time, so a slow test host could decide them (lead rule R5). DrainGate holds the queue until the registry is told a drain began (`AgentPaneStateFiles(drainBegan:)`), and records the order of the quit's steps: - QuitStepsStateFilesTests (both): a step that never awaits the state files never writes the draft, and the gate saw no drain. - theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles: the order is "deadline armed", then "drain began", and the exit ends with the draft on disk. - theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread: the main actor lets the held save go once the drain began; a wait that blocked it would reach its limit with the draft still queued.
…t in the test process) The green step 04224ccd at 07f1b95 crashed both quit suites before any assertion: AppServices.keyWindowSource reads `NSApp.keyWindow`, and `NSApp` is nil in a test process that never made NSApplication.shared; the quit steps ask for the key window (WindowManager.swift:143). The tests' services answer nil instead, with no app object.
…em off the main actor) The compile a1714da9 at d77dbff refused QuitStateFilesTests.swift:85: the drainBegan closure runs in the drain, off the main actor, and called DrainGateLog.append, main-actor by the test target's default isolation. Both helper classes are nonisolated now (a Mutex and semaphores inside).
|
Review (independent review subagent, arch-rise): SHIP at 57dfaa1 Scope: the delta 07f1b95..57dfaa1 (0aaaccd seam, 9355de1 tests, d77dbff key-window fix, 57dfaa1 isolation fix), on top of the earlier SHIP at 07f1b95.
Notes (do not block): if |
…cking wait has a time limit The green step eb20257d at 57dfaa1 crashed both quit suites before any assertion: a quit step reads `NSApp.isActive` (NotificationCenterService+ProgramStatusSeen.swift:57), and `NSApp` is nil in a test process with no app object. The one helper makes NSApplication.shared first, as the other AppServices tests do; the root fix of those reads is fix/arch-rise-appservices-no-nsapp-trap. theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread gets a 1 minute time limit, so a wait that never ends fails the test instead of the run.
…ition root too The second trap (#28275 green eb20257db2553e2e4a4dd378, NotificationCenterService+ProgramStatusSeen.swift:57): AppServices.init starts notifications, whose viewed-program-status observation calls viewedTabs, which read NSApp.isActive. Any test that built AppServices and then awaited (each quit step awaits) trapped. - AppServices.init takes appActiveSource with no default; AppDelegate passes { NSApp.isActive }. - The notifications' six reads (viewedTabs, isViewed, the program-status visibility, arrive, the hand-off and sandbox-run banners) ask services.appActiveSource(). - The test target's init reads the application only if the test made one (NSApp?.keyWindow, NSApp?.isActive ?? false), so tests behave as before and never unwrap the global. - AppServicesKeyWindowSourceTests: the recording test also checks the app-active lookups and awaits the started observation's first ask of the injected source (no clock).
…lose; a lost draft is named and told R9 review of #28275 (reviewer 0053c0be): - R-L1 (bare 2 s): the state files' drain has no limit of its own. The quit awaits it only after its deadline is armed: QuitCoordinator's engine stop now arms QuitDeadline.engineStopLimit (15 s), drains, closes the engines (`closeEngines`, a seam for tests) and drains again, so a page that closes and saves its draft (the composer's page-hide flush) is on disk when the quit ends. The forced quit awaits it under its 160 ms deadline. The prepareWindows drain is gone (the engine stop covers it). QueueDrain is deleted: each file awaits its own queue (`AgentPaneStateFile.saved()`). - R-L5 (silent loss): when a quit deadline ends the process while a state file still saves, its end (`AgentPaneStateLoss.deadlineEnd`) logs a fault naming the files (no content) and records them in `<state>/agent-state-lost.json`; the next launch takes the record and, for a new chat's unsent text, shows a toast ("Text typed in a new chat just before cmux quit was not saved.", 21 locales). Both deadline reasons name the state files. - Tests (no clock decides any): QuitStepsStateFilesTests (the engines close after the queued draft is on disk; a draft saved while the engines close is on disk when the quit ends), AgentPaneStateLossTests (the deadline records the files still saving, the next launch takes them once, the notice is only for a lost draft), AgentPaneStateFileTests (a file is saving until its saves ran). Not run here (no Swift build on this VM): the fleet runs them.
…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.
…ith it); PhoneAdmissionStoreTests need no forgetThisRun Fleet compile 6ade1fafcc18 at b062435: QuitCoordinator.swift:215 had no 'choice' in quitSteps(), and two feat tests called the static per-path forgetThisRun, which the per-file inForce record replaces: the restored store's new list file remembers no digest, as a relaunch does.
There was a problem hiding this comment.
8 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plans/cmux-next/threading-model.md">
<violation number="1" location="plans/cmux-next/threading-model.md:130">
P2: This update documents agent-pane state-file work without registering its queue, so the threading model still omits `com.cmuxterm.app.next.agent-pane.state-file`. Add that queue to the Agents registration list and describe its utility serial ownership.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateFile.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateFile.swift:46">
P2: The `defer` can move `ran` backwards when concurrent writes reach the serial queue out of number order. Keep this marker monotonic, otherwise a completed file remains reported as saving and can produce a false lost-draft notice at the next quit.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneStateLossTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneStateLossTests.swift:64">
P3: This test does not cover the either-order behavior it claims to verify: it always reads through `take()` before calling `lastQuitCutOffPairedPhones()`. Add a case that calls the paired check first, and exercise the notice read path as well.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedStore.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedStore.swift:110">
P2: This queues a read-only reload as a save, so a quit deadline that overlaps a reload can mark `paired-devices.json` cut off even though no save was interrupted. The next launch then clears the pairing seal and forces every phone to pair again; track read transactions separately from saves, or only mark the file when a save/withdrawal is actually pending.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift:313">
P1: This check is not atomic with admitting work to the file queue. A quit deadline can observe no save just before a removal is queued, exit without recording `paired-devices.json`, and leave the pre-removal paired list trusted after relaunch; synchronize deadline admission with queue accounting or close the queue before taking the cut-off snapshot.</violation>
<violation number="2" location="Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift:357">
P2: Ignoring marker-removal failure can leave a stale `paired-devices.cut-off` that invalidates every subsequent pairing on relaunch. Handle failed cleanup or make a later successful save retire the marker before allowing the marker to trigger recovery again.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextControl/Diagnostics/ForcedQuitExit.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextControl/Diagnostics/ForcedQuitExit.swift:40">
P1: The deadline callback performs blocking filesystem writes before `_exit`, so a stalled state directory can prevent the hard deadline from ending a stuck quit. Record the loss through a bounded/nonblocking path, or guarantee the deadline thread reaches the process exit independently of that recording.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateLoss.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateLoss.swift:71">
P2: `pairedCutOffSettled()` forgets the pending marker before knowing that record cleanup succeeded. If `write` fails, later settlement calls no-op and the record remains forever, causing repeated fail-closed recovery on every launch. Make the write report success and clear `state.pending` only after cleanup succeeds.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| /// when one is queued or running, it writes the cut-off marker. It never calls the Keychain (a hung | ||
| /// securityd must not hold the process's end): the next launch clears the seal (``distrustIfCutOff(recorded:)``). | ||
| public func markCutOffIfSaving() -> CutOff { | ||
| guard isSaving else { return .notSaving } |
There was a problem hiding this comment.
P1: This check is not atomic with admitting work to the file queue. A quit deadline can observe no save just before a removal is queued, exit without recording paired-devices.json, and leave the pre-removal paired list trusted after relaunch; synchronize deadline admission with queue accounting or close the queue before taking the cut-off snapshot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift, line 313:
<comment>This check is not atomic with admitting work to the file queue. A quit deadline can observe no save just before a removal is queued, exit without recording `paired-devices.json`, and leave the pre-removal paired list trusted after relaunch; synchronize deadline admission with queue accounting or close the queue before taking the cut-off snapshot.</comment>
<file context>
@@ -247,35 +280,104 @@ public struct MobileHostPairedDevicesFile: Sendable {
+ /// when one is queued or running, it writes the cut-off marker. It never calls the Keychain (a hung
+ /// securityd must not hold the process's end): the next launch clears the seal (``distrustIfCutOff(recorded:)``).
+ public func markCutOffIfSaving() -> CutOff {
+ guard isSaving else { return .notSaving }
+ do {
+ try writeMarker(cutOffMarker)
</file context>
| self.save = save | ||
| self.armDeadline = armDeadline ?? { QuitDeadline(reason: "a forced quit began its Home cache write").arm(after: $0) } | ||
| self.armDeadline = armDeadline ?? { | ||
| QuitDeadline(reason: "a forced quit began its last writes (the Home cache, the agent panes' state files)", end: deadlineEnd).arm(after: $0) |
There was a problem hiding this comment.
P1: The deadline callback performs blocking filesystem writes before _exit, so a stalled state directory can prevent the hard deadline from ending a stuck quit. Record the loss through a bounded/nonblocking path, or guarantee the deadline thread reaches the process exit independently of that recording.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextControl/Diagnostics/ForcedQuitExit.swift, line 40:
<comment>The deadline callback performs blocking filesystem writes before `_exit`, so a stalled state directory can prevent the hard deadline from ending a stuck quit. Record the loss through a bounded/nonblocking path, or guarantee the deadline thread reaches the process exit independently of that recording.</comment>
<file context>
@@ -25,16 +25,20 @@ public final class ForcedQuitExit {
self.save = save
- self.armDeadline = armDeadline ?? { QuitDeadline(reason: "a forced quit began its Home cache write").arm(after: $0) }
+ self.armDeadline = armDeadline ?? {
+ QuitDeadline(reason: "a forced quit began its last writes (the Home cache, the agent panes' state files)", end: deadlineEnd).arm(after: $0)
+ }
self.end = end ?? { exit(0) }
</file context>
| host's paired-phone list and its Keychain seal, utility, serial: its file and Keychain calls block this queue, | ||
| never main; one item per save, read or withdrawal, each from a person's action or an account change, so no back-pressure; | ||
| quit waits at most 2 s for it under `terminate`) | ||
| the quit awaits it holding no thread, with the agent panes' state files (`AppServices.stateFilesDrained`), after its deadline is armed; that deadline records the files still saving, `AgentPaneStateLoss`) |
There was a problem hiding this comment.
P2: This update documents agent-pane state-file work without registering its queue, so the threading model still omits com.cmuxterm.app.next.agent-pane.state-file. Add that queue to the Agents registration list and describe its utility serial ownership.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At plans/cmux-next/threading-model.md, line 130:
<comment>This update documents agent-pane state-file work without registering its queue, so the threading model still omits `com.cmuxterm.app.next.agent-pane.state-file`. Add that queue to the Agents registration list and describe its utility serial ownership.</comment>
<file context>
@@ -127,7 +127,7 @@ matches the label's literal text; `<x>` matches an interpolation). Its owner row
host's paired-phone list and its Keychain seal, utility, serial: its file and Keychain calls block this queue,
never main; one item per save, read or withdrawal, each from a person's action or an account change, so no back-pressure;
- quit waits at most 2 s for it under `terminate`)
+ the quit awaits it holding no thread, with the agent panes' state files (`AppServices.stateFilesDrained`), after its deadline is armed; that deadline records the files still saving, `AgentPaneStateLoss`)
- Browser: `com.cmuxterm.next.browser-host.provider.write`, `cmux browser-host provider reader`,
`com.cmuxterm.chromium-host.read`, `com.cmuxterm.chromium-host.exit`, `com.cmuxterm.app.next.cef-children`,
</file context>
| } | ||
| queue.async { [self] in | ||
| guard newest.withLock({ $0 == number }) else { return } | ||
| defer { numbers.withLock { $0.ran = number } } |
There was a problem hiding this comment.
P2: The defer can move ran backwards when concurrent writes reach the serial queue out of number order. Keep this marker monotonic, otherwise a completed file remains reported as saving and can produce a false lost-draft notice at the next quit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateFile.swift, line 46:
<comment>The `defer` can move `ran` backwards when concurrent writes reach the serial queue out of number order. Keep this marker monotonic, otherwise a completed file remains reported as saving and can produce a false lost-draft notice at the next quit.</comment>
<file context>
@@ -51,12 +38,13 @@ public nonisolated final class AgentPaneStateFile: Sendable {
}
queue.async { [self] in
- guard newest.withLock({ $0 == number }) else { return }
+ defer { numbers.withLock { $0.ran = number } }
+ guard numbers.withLock({ $0.newest == number }) else { return }
let data: Data?
</file context>
| defer { numbers.withLock { $0.ran = number } } | |
| defer { numbers.withLock { $0.ran = max($0.ran, number) } } |
| } | ||
| let selected = state.generation | ||
| MobileHostPairedDevicesFile.enqueue { [self] in | ||
| file.enqueue { [self] in |
There was a problem hiding this comment.
P2: This queues a read-only reload as a save, so a quit deadline that overlaps a reload can mark paired-devices.json cut off even though no save was interrupted. The next launch then clears the pairing seal and forces every phone to pair again; track read transactions separately from saves, or only mark the file when a save/withdrawal is actually pending.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedStore.swift, line 110:
<comment>This queues a read-only reload as a save, so a quit deadline that overlaps a reload can mark `paired-devices.json` cut off even though no save was interrupted. The next launch then clears the pairing seal and forces every phone to pair again; track read transactions separately from saves, or only mark the file when a save/withdrawal is actually pending.</comment>
<file context>
@@ -97,7 +107,7 @@ public final class MobileHostPairedStore: Sendable {
}
let selected = state.generation
- MobileHostPairedDevicesFile.enqueue { [self] in
+ file.enqueue { [self] in
var list = file.load(userID: user)
let removals = self.state.withLock { state in
</file context>
| } | ||
| return | ||
| } | ||
| try? FileManager.checked.removeItem(at: cutOffMarker) |
There was a problem hiding this comment.
P2: Ignoring marker-removal failure can leave a stale paired-devices.cut-off that invalidates every subsequent pairing on relaunch. Handle failed cleanup or make a later successful save retire the marker before allowing the marker to trigger recovery again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift, line 357:
<comment>Ignoring marker-removal failure can leave a stale `paired-devices.cut-off` that invalidates every subsequent pairing on relaunch. Handle failed cleanup or make a later successful save retire the marker before allowing the marker to trigger recovery again.</comment>
<file context>
@@ -247,35 +280,104 @@ public struct MobileHostPairedDevicesFile: Sendable {
+ }
+ return
+ }
+ try? FileManager.checked.removeItem(at: cutOffMarker)
+ cutOffSettled()
+ clearedForCutOff.withLock { $0 = true }
</file context>
| state.withLock { state in | ||
| guard !state.pending.isEmpty else { return } | ||
| let pending = state.pending | ||
| state.pending = [] |
There was a problem hiding this comment.
P2: pairedCutOffSettled() forgets the pending marker before knowing that record cleanup succeeded. If write fails, later settlement calls no-op and the record remains forever, causing repeated fail-closed recovery on every launch. Make the write report success and clear state.pending only after cleanup succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneStateLoss.swift, line 71:
<comment>`pairedCutOffSettled()` forgets the pending marker before knowing that record cleanup succeeded. If `write` fails, later settlement calls no-op and the record remains forever, causing repeated fail-closed recovery on every launch. Make the write report success and clear `state.pending` only after cleanup succeeds.</comment>
<file context>
@@ -0,0 +1,145 @@
+ state.withLock { state in
+ guard !state.pending.isEmpty else { return }
+ let pending = state.pending
+ state.pending = []
+ let onDisk = record.flatMap { try? Data(blockingContentsOf: $0) }.flatMap { try? JSONDecoder().decode([String].self, from: $0) } ?? []
+ write(onDisk.filter { !pending.contains($0) && !Self.isPaired($0) })
</file context>
| defer { try? FileManager.default.removeItem(at: directory) } | ||
| AgentPaneStateLoss(directory: directory).deadlineEnd(files: AgentPaneStateFiles(), alsoCutOff: { [AgentPaneStateLoss.pairedPhonesFileName] }, end: {})() | ||
| let launch = AgentPaneStateLoss(directory: directory) | ||
| let names = await launch.take() |
There was a problem hiding this comment.
P3: This test does not cover the either-order behavior it claims to verify: it always reads through take() before calling lastQuitCutOffPairedPhones(). Add a case that calls the paired check first, and exercise the notice read path as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneStateLossTests.swift, line 64:
<comment>This test does not cover the either-order behavior it claims to verify: it always reads through `take()` before calling `lastQuitCutOffPairedPhones()`. Add a case that calls the paired check first, and exercise the notice read path as well.</comment>
<file context>
@@ -0,0 +1,95 @@
+ defer { try? FileManager.default.removeItem(at: directory) }
+ AgentPaneStateLoss(directory: directory).deadlineEnd(files: AgentPaneStateFiles(), alsoCutOff: { [AgentPaneStateLoss.pairedPhonesFileName] }, end: {})()
+ let launch = AgentPaneStateLoss(directory: directory)
+ let names = await launch.take()
+ #expect(names == [AgentPaneStateLoss.pairedPhonesFileName])
+ #expect(launch.lastQuitCutOffPairedPhones(), "the paired check missed the record the notice read first")
</file context>
Bugbot needs on-demand usage enabledBugbot 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. |
…er test (the per-app writer registry) Compile 7b999930c9b2 at 0ff4259: UnsentDraftTests.swift built AgentUnsentDrafts(file:) with no files. Each test now gives its instances one AgentPaneStateFiles, so a fresh instance reads after the queued save as the static registry made it do. A grep of the package finds no other caller of a changed initializer without its new argument.
Bugbot needs on-demand usage enabledBugbot 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. |
|
Review (claude-review-subagent-cw): BLOCK at e79b754 Scope: Blocking finding
Checked and correct
Non-blocking findings
Observed / Not run
|
…he time they do not need Review of e79b754 (comment 6554069134): an unbounded drain before closeEngines could use up feat's engineCloseWait (a hung Keychain call on the paired-phones queue, a loaded disk), so CefShutdown lost its flush. The first drain is now a QuitStepLimiter step limited to the deadline's time left less the reply margin and engineCloseWait; past it, it runs beside the close and the quit awaits it in finishOverruns. The drain after the close is unchanged.
|
Review (claude-review-subagent-cw): SHIP at 2fdc8e5 Scope: The blocker from comment 6554069134 is fixed
Non-blocking findings
Observed / Not run
|
|
Conflict wave (lead decision 2026-11-01 05:20Z: "port onto feat"). This PR now keeps feat's #28204 drafts work and the #22919 quit limits, and it adds only what it adds beyond them:
Fleet evidence at 2fdc8e5:
|
feat-cmux-next landed the same goal as this PR's Swift change: #27749 saves the agent panes' drafts and scroll places off the main thread (one serial queue per file, latest wins), and #28275 gives each file one owner and has both quit paths await the saves under the quit deadline, holding no thread. The conflicts (12 files) resolve to the base: StateFileWriter, AgentPanePersistenceQuit, their tests and the App wiring are dropped, with the app_lines +8 allowance they needed. That also removes the two blocking findings of the review at 6aaafd0 (a seal that refused the quit's own late draft, and an unbounded drain before CefShutdown): the base never seals and bounds its wait. Kept: guardrails.py --sites (the whole-tree count) and its test. It now diffs against git's built-in empty tree instead of writing an empty tree and commit object (review note 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
This PR fixes the arch-ratchet tip rises of #27749 and #25535. These state files flush at quit and had no reviewed exception:
The statics move to instances. AppServices now owns one
AgentPaneStateFilesregistry and gives it toAgentUnsentDraftsandAgentPaneScrollAnchors. The registry rule is the same as before: one writer per path. The paired-phones file (MobileHostPairedDevicesFile) is now a class.IPhoneHostServiceowns one instance per state directory, and its queue and the record of the list in force are members of that instance.No quit blocks the main thread on these files. This PR removes all three
allowBlocking(.terminate)flushes. The quit awaits the files withAppServices.stateFilesDrained(). The wait holds no thread and has no limit of its own; the quit awaits it only after its deadline is armed:Forced quit (a second quit signal): before,
ForcedQuitExitprepareran its flush before the 160 ms deadline was armed, so it could wait 2 s. Now the exit arms the deadline first. Then, insave, it waits for the state files and for the Home cache write at the same time.Normal quit: before,
applicationWillTerminateandIPhoneHostService.stop()could wait 2 s each, so 4 s in all. NowQuitCoordinator's engine-stop step does this, in order:QuitDeadline.engineStopLimit(15 s).closeEngines, a seam for tests).The second drain catches a draft that a closing page saves (the composer's page-hide flush). The
prepareWindowsdrain and the bare 2 s limit are gone (R9 review, R-L1).A lost draft is named and told (R9 review, R-L5). If a quit deadline ends the process while a state file is still saving, the deadline's end (
AgentPaneStateLoss.deadlineEnd) does two things:.faultwith the file names only, never content.<state>/agent-state-lost.json.The next launch reads and deletes that record. If a new chat's unsent text was lost, it shows a toast: "Text typed in a new chat just before cmux quit was not saved." The string is localized in 21 locales. Both deadline reasons now name the state files. Before this PR, such a loss was silent. The paired-phones list is in the loss record too, and it fails closed, but the deadline's end never calls the Keychain. A hung securityd, the case
QuitDeadlineexists for, must not hold the process's end.MobileHostPairedDevicesFilecounts its saves and withdrawals that are queued or running.MobileHostPairedQuitWatchthat gives the deadline's end its current file from any thread.paired-devices.cut-off, beside the file), names the file in the fault and the record, and ends.paired-devices.cut-off (not written).AgentPaneStateLoss.lastQuitNames()), so this check and the notice share the same names in either order. Nothing stored counts in that run. It clears the Keychain seal, so the stored list no longer matches its seal and every phone must pair again (the digest rule ofload). A removed phone does not get its access back.pairedCutOffSettled()) once the fact is safe without it: its marker is on disk, or its seal is cleared.showNotices).AppServices.quitDeadlineEnd()builds the one deadline end that both quits use.One path for the drain.
QueueDrainis deleted. Each file awaits its own serial queue (AgentPaneStateFile.saved()), so the drain needs no timer and no race.Testable quit steps:
QuitCoordinator.steps()builds theQuitStepsthatcompleteruns.deadlineandcloseEnginesare replaceable, so a test runs the coordinator's own steps without ending its process or starting Chromium.Line budget:
SignalRelaymoves fromCmuxNextApp/QuittoCmuxNextControl/Diagnostics, next toForcedQuitExitandQuitDeadline. The loss record and its notice live in CmuxNextAgentPane. With this, CmuxNextApp's line count does not rise (+0).Sites fixed (paths under
Packages/macOS/CmuxNext/Sources/)CmuxNextAgentPane/AgentPaneStateFile.swift:31static let files = Mutex<...>CmuxNextMobile/MobileHost/MobileHostPairedDevices.swift:188static let queueCmuxNextMobile/MobileHost/MobileHostPairedDevices.swift:197static let inForceCmuxNextApp/AppDelegate.swift:42flush in ForcedQuitExitprepareCmuxNextApp/AppDelegate.swift:514flush inapplicationWillTerminateCmuxNextMobileHost/IPhoneHostService.swift:387flush instop()AppDelegate.swift:514was first listed as an exception candidate. An independent exception review then asked for it to be fixed with an async wait in the.terminateLaterphase, so this PR fixes it too.Ratchet delta (
arch_counts.py --delta <merge-base 5a3b7e1cef4> HEAD)All other rows: +0.
arch-ratchet.py --base origin/feat-cmux-next --head HEADsays "no architecture count rises". This PR does not changearch-ratchet.json(--updatewas not run).Testing
No clock decides any quit test. A test helper,
DrainGate, holds a state file's queue with a save, and only the start of a drain lets that save go:AgentPaneStateFiles(drainBegan:)is an injected init hook, not a static. The drain calls it as it begins, and the gate releases the held save from it (or from the test's owndefer, after its checks).The quit tests build their services through one helper,
QuitStateFilesTests.services. It makesNSApplication.sharedfirst, answers no key window, and puts in aQuitDeadlinewhose end does nothing. A test process has no app object; the root fix of that trap is its own PR,fix/arch-rise-appservices-no-nsapp-trap.QuitStateFilesTests.theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFiles: the step order is["deadline armed", "drain began"], and the draft is on disk when the forced exit ends.QuitStateFilesTests.theQuitAwaitsAQueuedDraftWithoutBlockingTheMainThread: the main actor releases the held save while the quit waits. A wait that blocked the main thread would never end, and.timeLimit(.minutes(1))fails the test.QuitStepsStateFilesTestsrunsQuitCoordinator's ownstopBrowserEngines:theEnginesCloseAfterTheQueuedDraftIsOnDisk: two drains happen, and the draft is on disk when the engines close.aDraftSavedWhileTheEnginesCloseIsOnDiskWhenTheQuitEnds: a draft queued during the engines' close is on disk when the step returns.AgentPaneStateLossTests:MobileHostPairingTests.aRemovalCutOffByAQuitDeadlineIsNotTrustedAfterARelaunch: a removal's save is held at the seal, before its own clear, when the deadline's end runs. The test checks four things:digest,setDigestandclearDigest.MobileHostPairingTests.aCutOffRemovalWithoutItsMarkerIsNotTrustedAfterARelaunch: an injected disk refuses the marker. The record names the file with the note, no marker is on disk, and the next launch, told only by the record, trusts no phone and clears the seal.MobileHostPairingTests.aRecordOnlyLaunchWhoseClearHangsStillLeavesTheListUntrusted: a launch told only by the record has its seal clear held. A quit deadline during that clear finds.marked, and the launch after it, with no record, trusts no phone.MobileHostPairingTests.aRecordOnlyLaunchWithNoMarkerAndARefusedClearKeepsTheRecord: a launch with no marker (the disk refuses it) and a refused clear never tells the record the fact is safe, and the launch after it still trusts no phone.AgentPaneStateLossTests.theRecordKeepsNamingACutOffPairedListUntilItIsSettled: after the launch reads the record, it names only the paired list. A deadline in that run keeps that name beside its own, and only settling removes it.MobileHostPairingTests.aRemovalTakesTheOldDigestAwayAfterTheSavesBeforeItandaRemovalNeverWaitsOnTheCallersThreadnow release their held save on a signal, after the removal is queued, not after 0.3 s or 0.6 s. The second no longer measures time: a removal that waited on the caller's thread would never return, and its time limit fails the test.AgentPaneStateLossTests.thePairedCheckAndTheNoticeShareOneReadOfTheRecord: the notice reads the record first, and the paired check still sees it.QuitStateFilesTests.theLossRecordNamesThePairedListByItsFileName: the record's paired-phones name matches the file's own name.AgentPaneStateFileTests: a file counts as saving until its saves ran;drained()waits for queued saves and holds no thread.MobileHostPairingTests: one file object per run.GatedSealbecameHeldSeal, which holds the same file's queue.ChatScrollRestore*,UnsentDraftTestsandRestoredNewTabPageTitleTestsbuild their own registry.Observed / Not run
prepush-gate.pyreports no new violations.arch-ratchet.pyreports no rises.lint-ios-package-conventions.sh --namespace-fixreports no violations.localization_catalog.py checkreports nothing for the new key.NSApp.keyWindow.NSApp.isActive.NSApplication.sharedin the test helper.CmuxToastCenter). Fixed in 96a1de8.blockingWrite). Fixed in e22c234. I checked every public declaration this PR adds or changes for default arguments or types from internal imports, and found no other case.fix/arch-rise-quit-flush-redproof-6) failed on exactly the 9 expected tests, each on its assertions, with no crash:theForcedQuitArmsItsDeadlineBeforeItWaitsForTheStateFilesQuitStepsStateFilesTeststeststheDeadlineRecordsTheFilesStillSavingAndTheNextLaunchTakesThemaCutOffPairedListIsRecordedAndToldthePairedCheckAndTheNoticeShareOneReadOfTheRecordaCutOffRemovalWithoutItsMarkerIsNotTrustedAfterARelaunchaRemovalCutOffByAQuitDeadlineIsNotTrustedAfterARelaunchaRecordOnlyLaunchWhoseClearHangsStillLeavesTheListUntrustedThe R9 review then found that the record was deleted at the launch's first read, so a failed marker write at launch plus a refused or hung clear could still lose the fact. Fixed in 626d97e, with the two timed tests made signal-driven.fix/arch-rise-quit-flush-redproof-7) and green ofQuitStateFilesTests,QuitStepsStateFilesTests,AgentPaneStateLossTestsandMobileHostPairingTests.Changelog
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes the arch-ratchet tip rises for the agent-pane state files (cmux#27749) and the paired-phones file (cmux#25535): each file now has one owner, and no quit blocks the main thread waiting on them.
AppServicesowns oneAgentPaneStateFilesregistry, andIPhoneHostServiceowns oneMobileHostPairedDevicesFileper state directory, keeping the one-writer-per-path rule.allowBlocking(.terminate)flushes. The forced quit arms its 160 ms deadline before awaiting the files;QuitCoordinator's engine stop arms its 15 s deadline, drains the files, closes the engines, and drains again so a closing page's saved draft is on disk. The first drain is bounded by the deadline's time left less the reply margin and the engine close's wait, so it can't starveCefShutdown; an overrun runs beside the close and is awaited before the quit ends. The drains hold no thread.<state>/agent-state-lost.json; the next launch toasts a lost unsent draft.QuitCoordinatorbuilds its steps viaquitSteps()with a replaceablecloseEnginesseam, so the quit tests run the coordinator's own engine stop with a queued draft.SignalRelaytoCmuxNextControl/Diagnosticsto keepCmuxNextApp's line count from rising.DrainGatethat the registry'sdrainBeganhook releases when a drain starts, so no clock decides them. The testAppServicesinitializer takes the state files so the tests can hold one, and their services answer no key window because a test process has noNSApp. Draft, scroll-restore, and title tests build their ownAgentPaneStateFiles, sharing one per test so a fresh instance reads after its queued saves, as the static registry made it do.Written for commit 2fdc8e5. Summary will update on new commits.
Takes back #27749: its quit flush blocked the main thread twice (
allowBlocking(.terminate) { AgentPaneStateFile.flushAll() }in ForcedQuitExitpreparebefore the 160 ms forced deadline was armed, and again in applicationWillTerminate) with no reviewed arch-ratchet exception (reward-hacking audit, arch-ratchet tip rises). The drafts are still saved. The quit now awaits the same files after its deadline is armed, with the main thread free. A draft that the deadline cuts off is recorded and told at the next launch.No capture: the one new UI is an existing toast (
CmuxToast) with new text. It shows only at the launch after a quit deadline ended the process while a draft was still saving, and a capture tour cannot reach that state without killing a quit mid-save.