test(macos): view model lifecycle/leak tests + timer teardown fixes #493
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import Foundation | ||
|
|
||
| /// Holds a repeating `Timer` outside of a `@MainActor` view model's isolation so | ||
| /// that the model's `deinit` (which is nonisolated and can run on whatever | ||
| /// thread drops the last reference) can tear the timer down without touching | ||
| /// actor-isolated state. | ||
| /// | ||
| /// A scheduled `Timer` is retained by its run loop, and our timers capture | ||
| /// `self` weakly, so the run loop would keep firing the timer even after the | ||
| /// view model is gone. Storing the timer here means it is invalidated when the | ||
| /// box is released (i.e. when the owning view model deallocates), so no | ||
| /// orphaned timer is left ticking on the main run loop — the leak/slowness | ||
| /// field reports point straight at these never-invalidated timers. | ||
| /// | ||
| /// Thread safety: the stored timer is guarded by a lock (a deinit on a | ||
| /// non-main thread may race main-thread access), and `Timer.invalidate()` is | ||
| /// only ever called on the main thread — the thread whose run loop scheduled | ||
| /// it — hopping via `DispatchQueue.main.async` when teardown originates | ||
| /// elsewhere. | ||
| final class TimerBox: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private var _timer: Timer? | ||
|
|
||
| /// The held timer. Access is lock-protected; schedule the timer itself on | ||
| /// the main run loop (all our callers are `@MainActor`). | ||
| var timer: Timer? { | ||
| get { | ||
| lock.lock(); defer { lock.unlock() } | ||
| return _timer | ||
| } | ||
| set { | ||
| lock.lock(); defer { lock.unlock() } | ||
| _timer = newValue | ||
| } | ||
| } | ||
|
|
||
| /// Whether a timer is currently held and scheduled. Reads false immediately | ||
| /// after `invalidate()`, even while an off-main teardown's actual | ||
| /// `Timer.invalidate()` call is still hopping to the main thread. | ||
| var isActive: Bool { | ||
| lock.lock(); defer { lock.unlock() } | ||
| return _timer?.isValid ?? false | ||
| } | ||
|
|
||
| /// Takes the timer out under the lock, then invalidates it on the main | ||
| /// thread — directly when already there, else via `DispatchQueue.main.async` | ||
| /// (`Timer.invalidate()` must run on the thread that scheduled the timer). | ||
| /// Safe to call repeatedly and from any thread. | ||
| func invalidate() { | ||
| lock.lock() | ||
| let timer = _timer | ||
| _timer = nil | ||
| lock.unlock() | ||
| guard let timer else { return } | ||
| if Thread.isMainThread { | ||
| timer.invalidate() | ||
| } else { | ||
| DispatchQueue.main.async { timer.invalidate() } | ||
| } | ||
| } | ||
|
|
||
| deinit { invalidate() } | ||
| } | ||
|
Comment on lines
+20
to
+63
There was a problem hiding this comment. Thread Safety and Data Race Issues in
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| import XCTest | ||
| import Foundation | ||
| @testable import BurnCore | ||
|
|
||
| /// A `UsageProvider` that counts `fetch()` calls and returns a minimal healthy | ||
| /// status, so `UsageViewModel`'s refresh loop can be driven without network. | ||
| final class CountingProvider: UsageProvider, @unchecked Sendable { | ||
| let name: ProviderName | ||
| private let lock = NSLock() | ||
| private var count = 0 | ||
|
|
||
| /// Number of times `fetch()` has been invoked, in a thread-safe read. | ||
| var fetchCount: Int { | ||
| lock.lock(); defer { lock.unlock() } | ||
| return count | ||
| } | ||
|
|
||
| init(name: ProviderName) { | ||
| self.name = name | ||
| } | ||
|
|
||
| func fetch() async -> ProviderStatus { | ||
| lock.lock(); count += 1; lock.unlock() | ||
| // Empty metrics keep `refresh()` on its happy path without needing the | ||
| // ledger, and make spend loading a no-op. | ||
| return ProviderStatus(provider: name, status: .ok, plan: "Test", metrics: [], message: nil) | ||
| } | ||
| } | ||
|
|
||
| /// Lifecycle / deallocation tests for the menu bar view models. These pin down | ||
| /// the memory-leak and background-slowness field reports: the refresh timers | ||
| /// must be torn down when a model stops or deallocates, rather than being left | ||
| /// firing forever on the main run loop. | ||
| @MainActor | ||
| final class ViewModelLifecycleTests: XCTestCase { | ||
| /// JSON that satisfies every `BurnLedger` query the models make (cost, | ||
| /// summary, and an empty time-series), so injected refreshes always succeed. | ||
| private let comboJSON = #"{"totalCost": {"total": 0}, "buckets": []}"# | ||
|
|
||
| override func setUp() { | ||
| super.setUp() | ||
| // Make `selectedProvider` deterministic (`.codex`) regardless of any | ||
| // value a developer's defaults might carry. | ||
| UserDefaults.standard.removeObject(forKey: "selectedProvider") | ||
| } | ||
|
|
||
| // MARK: - Helpers | ||
|
|
||
| /// A throwaway history store backed by a unique temp file, so tests never | ||
| /// touch the shared on-disk history. | ||
| private func makeTempHistory() -> UsageHistoryStore { | ||
| let url = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("burn-lifecycle-\(UUID().uuidString).json") | ||
| return UsageHistoryStore(fileURL: url) | ||
| } | ||
|
|
||
| /// Pumps the main run loop and the cooperative executor for `seconds` so | ||
| /// in-flight `Task`s settle and any due timer would get a chance to fire. | ||
| private func settle(_ seconds: TimeInterval) async { | ||
| let deadline = Date().addingTimeInterval(seconds) | ||
| while Date() < deadline { | ||
| RunLoop.main.run(until: Date().addingTimeInterval(0.02)) | ||
| await Task.yield() | ||
| } | ||
| } | ||
|
|
||
| /// Pumps the run loop until `condition` holds or `timeout` elapses. | ||
| @discardableResult | ||
| private func wait(timeout: TimeInterval, until condition: () -> Bool) async -> Bool { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while Date() < deadline { | ||
| if condition() { return true } | ||
| RunLoop.main.run(until: Date().addingTimeInterval(0.02)) | ||
| await Task.yield() | ||
| } | ||
| return condition() | ||
| } | ||
|
|
||
| // MARK: - LiveBurnViewModel | ||
|
|
||
| func testLiveBurnViewModelDeallocatesAfterStop() async { | ||
| let fake = FakeRunner(output: comboJSON) | ||
| weak var weakVM: LiveBurnViewModel? | ||
| do { | ||
| let vm = LiveBurnViewModel(ledger: BurnLedger(runner: fake)) | ||
| weakVM = vm | ||
| vm.start() | ||
| vm.stop() | ||
| } | ||
| await wait(timeout: 2) { weakVM == nil } | ||
| XCTAssertNil(weakVM, "LiveBurnViewModel leaked after start()/stop()") | ||
| } | ||
|
|
||
| /// Even when the view forgets to call `stop()` (e.g. an NSPopover close that | ||
| /// doesn't fire SwiftUI's `.onDisappear`), the `[weak self]` timer plus the | ||
| /// deinit teardown must let the model deallocate. | ||
| func testLiveBurnViewModelDeallocatesWithoutStop() async { | ||
| let fake = FakeRunner(output: comboJSON) | ||
| weak var weakVM: LiveBurnViewModel? | ||
| do { | ||
| let vm = LiveBurnViewModel(ledger: BurnLedger(runner: fake)) | ||
| weakVM = vm | ||
| vm.start() | ||
| // Deliberately no stop(). | ||
| } | ||
| await wait(timeout: 2) { weakVM == nil } | ||
| XCTAssertNil(weakVM, "LiveBurnViewModel leaked when stop() was never called") | ||
| } | ||
|
|
||
| func testLiveBurnViewModelStopsQueryingAfterStop() async { | ||
| let fake = FakeRunner(output: comboJSON) | ||
| let vm = LiveBurnViewModel(ledger: BurnLedger(runner: fake)) | ||
| vm.start() | ||
| // The immediate refresh queries every provider once (2 providers). | ||
| await wait(timeout: 3) { fake.capturedArgs.count >= 2 } | ||
| vm.stop() | ||
| XCTAssertFalse(vm.isRefreshTimerActive, "stop() left the refresh timer scheduled") | ||
| let callsAtStop = fake.capturedArgs.count | ||
| // Wait past one .m5 refresh interval (3s): a live timer would have fired. | ||
| await settle(3.5) | ||
| XCTAssertEqual(fake.capturedArgs.count, callsAtStop, | ||
| "runner kept being called after stop() — timer was not invalidated") | ||
| } | ||
|
|
||
| // MARK: - UsageViewModel | ||
|
|
||
| /// With `autostart: false` the model must not fetch or schedule anything, | ||
| /// and must deallocate immediately once released. | ||
| func testUsageViewModelNoSideEffectsWhenAutostartFalse() async { | ||
| let provider = CountingProvider(name: .codex) | ||
| let fake = FakeRunner(output: comboJSON) | ||
| weak var weakVM: UsageViewModel? | ||
| do { | ||
| let vm = UsageViewModel( | ||
| providers: [.codex: provider, .claude: CountingProvider(name: .claude)], | ||
| history: makeTempHistory(), | ||
| ledger: BurnLedger(runner: fake), | ||
| autostart: false | ||
| ) | ||
| weakVM = vm | ||
| XCTAssertFalse(vm.isRefreshTimerActive, "autostart:false must not schedule a timer") | ||
| } | ||
| await wait(timeout: 2) { weakVM == nil } | ||
| XCTAssertNil(weakVM, "UsageViewModel leaked even with autostart:false") | ||
| XCTAssertEqual(provider.fetchCount, 0, "provider was fetched despite autostart:false") | ||
| XCTAssertEqual(fake.capturedArgs.count, 0, "ledger was queried despite autostart:false") | ||
| } | ||
|
|
||
| func testUsageViewModelDeallocatesAfterStartStop() async { | ||
| let provider = CountingProvider(name: .codex) | ||
| let fake = FakeRunner(output: comboJSON) | ||
| weak var weakVM: UsageViewModel? | ||
| do { | ||
| let vm = UsageViewModel( | ||
| providers: [.codex: provider, .claude: CountingProvider(name: .claude)], | ||
| history: makeTempHistory(), | ||
| ledger: BurnLedger(runner: fake), | ||
| autostart: false | ||
| ) | ||
| weakVM = vm | ||
| vm.start() | ||
| await settle(0.3) // let the immediate refresh + spend load settle | ||
| vm.stop() | ||
| } | ||
| await wait(timeout: 2) { weakVM == nil } | ||
| XCTAssertNil(weakVM, "UsageViewModel leaked after start()/stop()") | ||
| } | ||
|
|
||
| func testUsageViewModelTimerStopsOnStop() async { | ||
| let provider = CountingProvider(name: .codex) | ||
| let vm = UsageViewModel( | ||
| providers: [.codex: provider, .claude: CountingProvider(name: .claude)], | ||
| history: makeTempHistory(), | ||
| ledger: BurnLedger(runner: FakeRunner(output: comboJSON)), | ||
| autostart: false | ||
| ) | ||
| vm.start() | ||
| XCTAssertTrue(vm.isRefreshTimerActive, "start() did not schedule the refresh timer") | ||
| await wait(timeout: 2) { provider.fetchCount >= 1 } // immediate refresh fetched once | ||
| vm.stop() | ||
| XCTAssertFalse(vm.isRefreshTimerActive, "stop() did not invalidate the refresh timer") | ||
| let fetchesAtStop = provider.fetchCount | ||
| await settle(0.5) | ||
| XCTAssertEqual(provider.fetchCount, fetchesAtStop, "provider fetched again after stop()") | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.