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 ) 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() + } +}