diff --git a/apps/macos/Sources/Burn/LiveBurnViewModel.swift b/apps/macos/Sources/Burn/LiveBurnViewModel.swift index 29fae6c..b2457dd 100644 --- a/apps/macos/Sources/Burn/LiveBurnViewModel.swift +++ b/apps/macos/Sources/Burn/LiveBurnViewModel.swift @@ -102,7 +102,11 @@ final class LiveBurnViewModel: ObservableObject { private let providers = ProviderName.allCases private let ledger: BurnLedger - private var timer: Timer? + /// The refresh timer, held in a box so `deinit` (nonisolated) can invalidate + /// it without touching `@MainActor` state — a backstop for when the view's + /// `.onDisappear` doesn't fire (e.g. an NSPopover closing) and `stop()` is + /// never called. + private let timerBox = TimerBox() private var refreshing = false /// Set when a refresh is requested while one is in flight, so the running /// one does another pass (for the latest range) instead of being dropped. @@ -116,7 +120,7 @@ final class LiveBurnViewModel: ObservableObject { /// Begins the refresh loop and the background ingest watch. Idempotent. func start() { - guard timer == nil else { return } + guard timerBox.timer == nil else { return } Task { await ledger.startIngestWatch() } Task { await refresh() } scheduleTimer() @@ -124,8 +128,20 @@ final class LiveBurnViewModel: ObservableObject { /// Stops the loop and the watch. func stop() { - timer?.invalidate() - timer = nil + timerBox.invalidate() + Task { await ledger.stopIngestWatch() } + } + + /// Test hook: whether the refresh timer is currently scheduled. + var isRefreshTimerActive: Bool { timerBox.isActive } + + deinit { + timerBox.invalidate() + // Also stop the long-lived `burn ingest --watch` child, which would + // otherwise outlive the model when `stop()` never ran (e.g. a missed + // `.onDisappear`). Capture the ledger into a local — deinit may read + // stored properties but must not touch actor-isolated state directly. + let ledger = self.ledger Task { await ledger.stopIngestWatch() } } @@ -147,13 +163,13 @@ final class LiveBurnViewModel: ObservableObject { guard newRange != range else { return } range = newRange series = [:] - if timer != nil { scheduleTimer() } // only retime when running + if timerBox.timer != nil { scheduleTimer() } // only retime when running Task { await refresh() } } private func scheduleTimer() { - timer?.invalidate() - timer = Timer.scheduledTimer(withTimeInterval: range.refreshInterval, repeats: true) { [weak self] _ in + timerBox.invalidate() + timerBox.timer = Timer.scheduledTimer(withTimeInterval: range.refreshInterval, repeats: true) { [weak self] _ in Task { await self?.refresh() } } } diff --git a/apps/macos/Sources/Burn/TimerBox.swift b/apps/macos/Sources/Burn/TimerBox.swift new file mode 100644 index 0000000..572dc21 --- /dev/null +++ b/apps/macos/Sources/Burn/TimerBox.swift @@ -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() } +} diff --git a/apps/macos/Sources/Burn/UsageViewModel.swift b/apps/macos/Sources/Burn/UsageViewModel.swift index 72c642c..c6438a2 100644 --- a/apps/macos/Sources/Burn/UsageViewModel.swift +++ b/apps/macos/Sources/Burn/UsageViewModel.swift @@ -29,7 +29,10 @@ final class UsageViewModel: ObservableObject { let refreshInterval: TimeInterval = 60 - private var timer: Timer? + /// The repeating refresh timer, held in a box so `deinit` (nonisolated) can + /// invalidate it without touching `@MainActor` state. Previously the timer + /// was never invalidated, so a closed popover left it firing forever. + private let timerBox = TimerBox() /// While set, scheduled (non-forced) refreshes are skipped to let a 429 clear. private var backoffUntil: Date? private var consecutiveRateLimits = 0 @@ -86,13 +89,30 @@ final class UsageViewModel: ObservableObject { menuBarIcon = MenuBarIcon.render(usage: usage, offTarget: offTarget) } - private func start() { + /// Kicks off the first refresh and starts the repeating refresh timer. + /// Idempotent: a second call while the timer is live is a no-op. Internal + /// (not private) so lifecycle tests can drive it with `autostart: false`. + func start() { + guard timerBox.timer == nil else { return } Task { await refresh(force: true) } - timer = Timer.scheduledTimer(withTimeInterval: refreshInterval, repeats: true) { [weak self] _ in + timerBox.timer = Timer.scheduledTimer(withTimeInterval: refreshInterval, repeats: true) { [weak self] _ in Task { await self?.refresh() } } } + /// Invalidates the refresh timer. Call when the model is no longer needed so + /// no orphaned timer keeps firing on the main run loop. `deinit` also does + /// this as a backstop when the view forgets (an NSPopover close may not fire + /// SwiftUI's `.onDisappear`). + func stop() { + timerBox.invalidate() + } + + /// Test hook: whether the repeating refresh timer is currently scheduled. + var isRefreshTimerActive: Bool { timerBox.isActive } + + deinit { timerBox.invalidate() } + func select(_ provider: ProviderName) { guard provider != selectedProvider else { return } selectedProvider = provider diff --git a/apps/macos/Tests/BurnTests/ViewModelLifecycleTests.swift b/apps/macos/Tests/BurnTests/ViewModelLifecycleTests.swift new file mode 100644 index 0000000..7c136c7 --- /dev/null +++ b/apps/macos/Tests/BurnTests/ViewModelLifecycleTests.swift @@ -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()") + } +}