From 0faeb27727f587bbf8dbd208048df4926edba60f Mon Sep 17 00:00:00 2001 From: teamleaderleo Date: Sat, 26 Sep 2026 09:51:00 -0400 Subject: [PATCH 1/2] test: autosave defaults marker writes must not post no-op change notifications Session autosave clears the crash-only snapshot removal marker on every write. UserDefaults posts didChangeNotification even when the value does not change, so each autosave wakes every defaults observer in the app. This test fails until the marker helpers skip no-op writes. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../CrashDiagnosticSessionPolicyTests.swift | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/cmuxTests/CrashDiagnosticSessionPolicyTests.swift b/cmuxTests/CrashDiagnosticSessionPolicyTests.swift index 4a0cff5c6db2..48d3347cf8d5 100644 --- a/cmuxTests/CrashDiagnosticSessionPolicyTests.swift +++ b/cmuxTests/CrashDiagnosticSessionPolicyTests.swift @@ -424,6 +424,40 @@ struct CrashDiagnosticSessionPolicyTests { #expect(!AppDelegate.hasCrashOnlyPrimarySnapshotRemovalMarker(defaults: defaults)) } + /// Session autosave clears this marker on every write. `UserDefaults` posts + /// `didChangeNotification` even for no-op writes, which wakes every defaults + /// observer in the app (including SwiftUI's `@AppStorage` observer, which + /// takes SwiftUI's global update lock). A steady-state write must stay silent. + @Test + func crashOnlyPrimarySnapshotRemovalMarkerSkipsNoOpDefaultsWrites() throws { + let defaultsSuiteName = "CrashDiagnosticSessionPolicyTests.\(UUID().uuidString)" + let defaults = try #require(UserDefaults(suiteName: defaultsSuiteName)) + defer { + UserDefaults.standard.removePersistentDomain(forName: defaultsSuiteName) + } + let counter = DefaultsChangeCounter() + let observer = NotificationCenter.default.addObserver( + forName: UserDefaults.didChangeNotification, + object: defaults, + queue: nil + ) { _ in counter.increment() } + defer { NotificationCenter.default.removeObserver(observer) } + + AppDelegate.clearCrashOnlyPrimarySnapshotRemovalMarker(defaults: defaults) + #expect(counter.value == 0) + + AppDelegate.markCrashOnlyPrimarySnapshotRemoval(defaults: defaults) + #expect(counter.value == 1) + AppDelegate.markCrashOnlyPrimarySnapshotRemoval(defaults: defaults) + #expect(counter.value == 1) + + AppDelegate.clearCrashOnlyPrimarySnapshotRemovalMarker(defaults: defaults) + #expect(counter.value == 2) + AppDelegate.clearCrashOnlyPrimarySnapshotRemovalMarker(defaults: defaults) + #expect(counter.value == 2) + #expect(!AppDelegate.hasCrashOnlyPrimarySnapshotRemovalMarker(defaults: defaults)) + } + @Test func missingPrimaryRecoveryRequiresAnUncleanLaunchSignal() { #expect( @@ -606,3 +640,21 @@ struct CrashDiagnosticSessionPolicyTests { return url } } + +/// Counts `UserDefaults.didChangeNotification` deliveries from a synchronous observer. +private final class DefaultsChangeCounter: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + var value: Int { + lock.lock() + defer { lock.unlock() } + return count + } + + func increment() { + lock.lock() + count += 1 + lock.unlock() + } +} From 233ef82df77c99b0d55fd8a9b81b23c3f217088b Mon Sep 17 00:00:00 2001 From: teamleaderleo Date: Sat, 26 Sep 2026 09:51:48 -0400 Subject: [PATCH 2/2] perf: skip no-op UserDefaults writes on every session autosave Each autosave write removed a legacy geometry key, rewrote the window geometry data, and removed the crash-only snapshot marker. UserDefaults posts didChangeNotification for every set/remove, including no-ops, so each save ran every defaults observer on the persistence queue and enqueued a main-actor refresh for each addUserDefaultsObserver client. SwiftUI's @AppStorage observer also takes SwiftUI's global update lock there, contending with main-thread rendering (sampled at 209 and 83 blocked samples in _MovableLockLock on com.cmuxterm.app.sessionPersistence). Write defaults only when the stored value changes, via new change-only helpers in CmuxFoundation, and encode the geometry with sorted keys so the byte comparison is stable. Steady-state autosaves now post zero defaults notifications instead of three. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../UserDefaults+ChangeOnlyWrites.swift | 42 ++++++++++ .../UserDefaultsChangeOnlyWritesTests.swift | 76 +++++++++++++++++++ ...Delegate+CrashSessionSnapshotRemoval.swift | 6 +- Sources/AppDelegate.swift | 14 +++- 4 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/UserDefaults+ChangeOnlyWrites.swift create mode 100644 Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/UserDefaultsChangeOnlyWritesTests.swift diff --git a/Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/UserDefaults+ChangeOnlyWrites.swift b/Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/UserDefaults+ChangeOnlyWrites.swift new file mode 100644 index 000000000000..50ec9f26a36c --- /dev/null +++ b/Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/UserDefaults+ChangeOnlyWrites.swift @@ -0,0 +1,42 @@ +public import Foundation + +/// Writes that stay silent when the stored value already matches. +/// +/// `UserDefaults.set(_:forKey:)` and `removeObject(forKey:)` post +/// `UserDefaults.didChangeNotification` on every call, including writes that +/// store an identical value and removals of absent keys. That notification runs +/// every defaults observer in the process on the writing thread, including +/// SwiftUI's `@AppStorage` observer, which takes SwiftUI's global update lock +/// and so contends with main-thread rendering. Periodic writers (session +/// autosave) use these helpers so steady-state saves post nothing. +/// +/// The comparison reads through the normal search list, so a matching value in +/// the argument or registration domain also counts as unchanged. +extension UserDefaults { + /// Stores `value` for `key` unless the current value is byte-for-byte equal. + /// - Returns: `true` when a write happened. + @discardableResult + public func setIfChanged(_ value: Data, forKey key: String) -> Bool { + if data(forKey: key) == value { return false } + set(value, forKey: key) + return true + } + + /// Stores `value` for `key` unless the current value is the same Boolean. + /// - Returns: `true` when a write happened. + @discardableResult + public func setIfChanged(_ value: Bool, forKey key: String) -> Bool { + if let current = object(forKey: key) as? Bool, current == value { return false } + set(value, forKey: key) + return true + } + + /// Removes `key` only when a value is present. + /// - Returns: `true` when a removal happened. + @discardableResult + public func removeObjectIfPresent(forKey key: String) -> Bool { + guard object(forKey: key) != nil else { return false } + removeObject(forKey: key) + return true + } +} diff --git a/Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/UserDefaultsChangeOnlyWritesTests.swift b/Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/UserDefaultsChangeOnlyWritesTests.swift new file mode 100644 index 000000000000..e98e64974046 --- /dev/null +++ b/Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/UserDefaultsChangeOnlyWritesTests.swift @@ -0,0 +1,76 @@ +import Foundation +import Testing + +@testable import CmuxFoundation + +/// Behavior tests for the change-only `UserDefaults` writers: each no-op write +/// must post no `didChangeNotification`, and real changes still post exactly one. +@Suite struct UserDefaultsChangeOnlyWritesTests { + @Test func dataWriteIsSilentWhenUnchanged() throws { + try withIsolatedDefaults { defaults, counter in + let payload = Data("geometry".utf8) + #expect(defaults.setIfChanged(payload, forKey: "k")) + #expect(counter.value == 1) + #expect(!defaults.setIfChanged(payload, forKey: "k")) + #expect(counter.value == 1) + #expect(defaults.setIfChanged(Data("moved".utf8), forKey: "k")) + #expect(counter.value == 2) + #expect(defaults.data(forKey: "k") == Data("moved".utf8)) + } + } + + @Test func boolWriteIsSilentWhenUnchanged() throws { + try withIsolatedDefaults { defaults, counter in + #expect(defaults.setIfChanged(true, forKey: "flag")) + #expect(!defaults.setIfChanged(true, forKey: "flag")) + #expect(counter.value == 1) + #expect(defaults.setIfChanged(false, forKey: "flag")) + #expect(counter.value == 2) + #expect(defaults.object(forKey: "flag") as? Bool == false) + } + } + + @Test func removalIsSilentWhenAbsent() throws { + try withIsolatedDefaults { defaults, counter in + #expect(!defaults.removeObjectIfPresent(forKey: "missing")) + #expect(counter.value == 0) + defaults.set("v", forKey: "present") + #expect(defaults.removeObjectIfPresent(forKey: "present")) + #expect(counter.value == 2) + #expect(defaults.object(forKey: "present") == nil) + } + } + + private func withIsolatedDefaults( + _ body: (UserDefaults, NotificationTally) throws -> Void + ) throws { + let suiteName = "UserDefaultsChangeOnlyWritesTests.\(UUID().uuidString)" + let defaults = try #require(UserDefaults(suiteName: suiteName)) + defer { UserDefaults.standard.removePersistentDomain(forName: suiteName) } + let counter = NotificationTally() + let observer = NotificationCenter.default.addObserver( + forName: UserDefaults.didChangeNotification, + object: defaults, + queue: nil + ) { _ in counter.increment() } + defer { NotificationCenter.default.removeObserver(observer) } + try body(defaults, counter) + } +} + +private final class NotificationTally: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + var value: Int { + lock.lock() + defer { lock.unlock() } + return count + } + + func increment() { + lock.lock() + count += 1 + lock.unlock() + } +} diff --git a/Sources/AppDelegate+CrashSessionSnapshotRemoval.swift b/Sources/AppDelegate+CrashSessionSnapshotRemoval.swift index c11b9724d351..955454df2e8e 100644 --- a/Sources/AppDelegate+CrashSessionSnapshotRemoval.swift +++ b/Sources/AppDelegate+CrashSessionSnapshotRemoval.swift @@ -1,3 +1,4 @@ +import CmuxFoundation import Foundation extension AppDelegate { @@ -52,7 +53,7 @@ extension AppDelegate { nonisolated static func markCrashOnlyPrimarySnapshotRemoval( defaults: UserDefaults = .standard ) { - defaults.set(true, forKey: crashOnlyPrimarySnapshotRemovalDefaultsKey) + defaults.setIfChanged(true, forKey: crashOnlyPrimarySnapshotRemovalDefaultsKey) } nonisolated static func hasCrashOnlyPrimarySnapshotRemovalMarker( @@ -64,6 +65,7 @@ extension AppDelegate { nonisolated static func clearCrashOnlyPrimarySnapshotRemovalMarker( defaults: UserDefaults = .standard ) { - defaults.removeObject(forKey: crashOnlyPrimarySnapshotRemovalDefaultsKey) + // Called on every autosave write; skip the no-op removal notification. + defaults.removeObjectIfPresent(forKey: crashOnlyPrimarySnapshotRemovalDefaultsKey) } } diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 8eb0cf74b28c..e33a174d272c 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -3687,7 +3687,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent guard let data = Self.encodedPersistedWindowGeometryData(frame: frame, display: display) else { return } - defaults.set(data, forKey: Self.persistedWindowGeometryDefaultsKey) + defaults.setIfChanged(data, forKey: Self.persistedWindowGeometryDefaultsKey) } private nonisolated static func encodedPersistedWindowGeometryData( @@ -3700,7 +3700,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent frame: frame, display: display ) - return try? JSONEncoder().encode(payload) + // Sorted keys keep the bytes stable so autosave can skip unchanged writes. + let encoder = JSONEncoder() + encoder.outputFormatting = [.sortedKeys] + return try? encoder.encode(payload) } nonisolated static func decodedPersistedWindowGeometryData(_ data: Data) -> PersistedWindowGeometry? { @@ -3714,7 +3717,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent private nonisolated static func removeLegacyPersistedWindowGeometry( defaults: UserDefaults = .standard ) { - legacyPersistedWindowGeometryDefaultsKeys.forEach { defaults.removeObject(forKey: $0) } + legacyPersistedWindowGeometryDefaultsKeys.forEach { defaults.removeObjectIfPresent(forKey: $0) } } private func persistWindowGeometry(from window: NSWindow?) { @@ -5046,9 +5049,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent // Persistence can outlive its main-actor owner; retain only the Sendable // store so finishing a write cannot destroy AppDelegate on this queue. let writeBlock = { [sessionSnapshotStore] in + // Autosave runs every few seconds. Only write defaults that changed: + // each set/remove posts didChangeNotification even when it is a no-op, + // waking every defaults observer and SwiftUI's @AppStorage lock. Self.removeLegacyPersistedWindowGeometry() if let persistedGeometryData { - UserDefaults.standard.set( + UserDefaults.standard.setIfChanged( persistedGeometryData, forKey: Self.persistedWindowGeometryDefaultsKey )