bug fixes for reload/memory leaks

src/tui.zig

@@ -4,7 +4,6 @@ const zfin = @import("root.zig");
 const fmt = @import("format.zig");
 const Money = @import("Money.zig");
 const cli = @import("commands/common.zig");
-const portfolio_loader = @import("portfolio_loader.zig");
 const stderr = @import("stderr.zig");
 const keybinds = @import("tui/keybinds.zig");
 const tab_framework = @import("tui/tab_framework.zig");
@@ -378,19 +377,10 @@ pub const SymbolData = struct {
     pub fn clear(self: *SymbolData, allocator: std.mem.Allocator) void {
         if (self.candles) |c| allocator.free(c);
         if (self.dividends) |d| zfin.Dividend.freeSlice(allocator, d);
-        if (self.etf_profile) |profile| {
-            if (profile.holdings) |h| {
-                for (h) |holding| {
-                    if (holding.symbol) |s| allocator.free(s);
-                    allocator.free(holding.name);
-                }
-                allocator.free(h);
-            }
-            if (profile.sectors) |s| {
-                for (s) |sec| allocator.free(sec.name);
-                allocator.free(s);
-            }
-        }
+        // Use EtfProfile.deinit so we free every owned field —
+        // including `symbol` and `name`, which an earlier
+        // hand-rolled inline cleanup here was silently leaking.
+        if (self.etf_profile) |profile| profile.deinit(allocator);
         self.* = .{};
     }
 

src/tui/analysis_tab.zig

@@ -121,8 +121,16 @@ pub const tab = struct {
     /// PortfolioData and are reset by pd's own load path —
     /// nothing to free here for those.
     ///
-    /// Eagerly recomputes only if this tab is currently active —
-    /// otherwise next `activate` will lazy-load.
+    /// Deliberately does NOT eager-rebuild, even when this tab
+    /// is active. The reload broadcast fires BEFORE pd.reload
+    /// resets the portfolio arena; rebuilding now would store
+    /// borrowed pointers (e.g. BreakdownItem.label borrowing
+    /// from ClassificationEntry.bucket) that get freed seconds
+    /// later when the arena resets, producing use-after-free
+    /// crashes on the next draw. The orchestrator
+    /// (portfolio_tab.reloadPortfolioFile) calls
+    /// `app.loadTabData()` AFTER pd.reload completes, which
+    /// re-activates the active tab against fresh data.
     pub fn onPortfolioReload(state: *State, app: *App) void {
         if (state.result) |*ar| ar.deinit(app.allocator);
         state.result = null;
@@ -130,9 +138,6 @@ pub const tab = struct {
         // reload's broadcast already invalidated it via pd's own
         // reset path. Nothing to free here.
         state.loaded = false;
-        if (app.active_tab == .analysis) {
-            tab.activate(state, app) catch |err| std.log.debug("analysis activate failed: {t}", .{err});
-        }
     }
 };
 
@@ -509,6 +514,29 @@ test "tab.init produces zero-defaulted state" {
     try testing.expectEqual(zfin.analysis.Granularity.mid, state.sector_granularity);
 }
 
+test "onPortfolioReload clears state without eager rebuild" {
+    // Regression for the use-after-free crash where eager-rebuild
+    // inside onPortfolioReload stored borrowed pointers from the
+    // about-to-be-freed portfolio arena. Contract: this hook
+    // MUST clear state and set loaded=false, but MUST NOT call
+    // activate() or otherwise rebuild from app.portfolio (which
+    // is about to be reset by pd.reload).
+    //
+    // We can't easily verify "no eager rebuild" mechanically
+    // without a full App harness, but we CAN verify the
+    // post-conditions are exactly state-cleared.
+    var state: State = .{
+        .loaded = true,
+        // result stays null — non-null would need allocator to free.
+    };
+    var dummy_app: tui.App = undefined; // not touched when result is null
+
+    tab.onPortfolioReload(&state, &dummy_app);
+
+    try testing.expectEqual(false, state.loaded);
+    try testing.expect(state.result == null);
+}
+
 test "handleAction cycles sector granularity coarse → mid → fine → coarse" {
     var state: State = .{};
     var dummy_app: tui.App = undefined; // handleAction doesn't touch app

src/tui/history_tab.zig

@@ -215,6 +215,33 @@ pub const tab = struct {
         loadData(state, app);
     }
 
+    /// Drop cached timeline and compare view on portfolio reload.
+    /// `state.tl` borrows symbol strings from the previous
+    /// portfolio's memory, and `compare_resources.then_live_map`
+    /// / `now_live_map` borrow keys from `app.portfolio` — all
+    /// of which `pd.reload` is about to free. Drop them now.
+    ///
+    /// Deliberately does NOT eager-rebuild, even when this tab
+    /// is active. The reload broadcast fires BEFORE pd.reload
+    /// resets the portfolio arena; rebuilding now would store
+    /// borrowed pointers that get freed seconds later. The
+    /// orchestrator (portfolio_tab.reloadPortfolioFile) calls
+    /// `app.loadTabData()` AFTER pd.reload completes, which
+    /// re-activates the active tab against fresh data.
+    ///
+    /// Selections + cursor are reset because the row indices
+    /// they refer to may no longer be valid after the new
+    /// timeline is loaded (different snapshot count, etc.).
+    pub fn onPortfolioReload(state: *State, app: *App) void {
+        freeLoaded(state, app);
+        state.loaded = false;
+        state.cursor = 0;
+        state.selections = .{ null, null };
+        state.table_first_line = 0;
+        state.table_row_count = 0;
+        state.expanded_buckets.clearRetainingCapacity();
+    }
+
     pub const tick = framework.noopTick(State);
 
     pub fn handleAction(state: *State, app: *App, action: Action) void {

src/tui/performance_tab.zig

@@ -157,7 +157,22 @@ fn loadData(state: *State, app: *App) void {
         app.symbol_data.etf_loaded = true;
         if (app.svc.getEtfProfile(app.symbol, .{})) |etf_result| {
             if (etf_result.data.isEtf()) {
+                // Take ownership of the EtfProfile data. We
+                // deliberately don't call etf_result.deinit
+                // here — the data fields (symbol, name,
+                // holdings, sectors) are now owned by
+                // symbol_data and will be freed by
+                // symbol_data.clear() on next symbol change.
+                // The FetchResult wrapper itself is a struct
+                // with no separate heap allocation; dropping
+                // it leaks nothing.
                 app.symbol_data.etf_profile = etf_result.data;
+            } else {
+                // Non-ETF: the profile data was still allocated
+                // by getEtfProfile (symbol dupe at minimum,
+                // possibly name from Wikidata fallback). Free
+                // it via the FetchResult contract.
+                etf_result.deinit();
             }
         } else |_| {}
     }

src/tui/portfolio_tab.zig

@@ -1755,6 +1755,15 @@ pub fn buildWelcomeScreenLines(
 /// Goes through the same `loadPortfolioFromPaths` the initial
 /// load uses, so a manual reload sees the merged view of every
 /// `portfolio*.srf` in the resolved directory — same as the CLI.
+///
+/// Lifecycle: broadcast onPortfolioReload BEFORE pd.reload so
+/// every tab clears derived state that borrows from the
+/// portfolio arena. Tabs MUST NOT eager-rebuild from those
+/// hooks — pd.reload is about to free the arena out from under
+/// them. Instead, after pd.reload completes, app.loadTabData()
+/// re-activates the currently-active tab against fresh data.
+/// Inactive tabs stay in `loaded = false` and lazy-rebuild on
+/// next switch.
 pub fn reloadPortfolioFile(state: *State, app: *App) void {
     if (app.portfolio.paths.len == 0) {
         app.setStatus("No portfolio file to reload");
@@ -1766,6 +1775,7 @@ pub fn reloadPortfolioFile(state: *State, app: *App) void {
     // models, cursor / expansion indices that pointed into the
     // about-to-be-freed portfolio data). MUST happen BEFORE
     // pd.reload starts freeing/recreating the underlying data.
+    // Tabs MUST NOT eager-rebuild from this hook (see above).
     app.broadcast("onPortfolioReload", .{});
 
     // Reload watchlist file too (if separate). pd doesn't read
@@ -1794,14 +1804,24 @@ pub fn reloadPortfolioFile(state: *State, app: *App) void {
     };
     if (app.portfolio.summary == null) return;
 
-    // Activate this tab's UI rebuild now that the data is
-    // ready. (onPortfolioReload above already cleared the old
-    // UI state; this re-derives the new state.)
+    // Always rebuild portfolio_tab UI state (regardless of
+    // which tab is active) — the onPortfolioReload broadcast
+    // cleared it above, and if we leave it empty the user
+    // could switch back without an activate firing in some
+    // paths. Cheap (sort + filter on ~30 holdings).
     sortPortfolioAllocations(state, app);
     buildAccountList(state, app);
     recomputeFilteredPositions(state, app);
     rebuildPortfolioRows(state, app);
 
+    // Re-activate the currently-active tab (whatever it is) so
+    // it rebuilds derived state from the freshly-reloaded
+    // portfolio data. Tabs that gate on `state.loaded` already
+    // got `loaded = false` from the broadcast above, so this
+    // call triggers their full re-load. For portfolio_tab
+    // itself this is idempotent with the manual rebuild above.
+    app.loadTabData();
+
     app.setStatus("Portfolio reloaded from disk");
 }
 

src/tui/projections_tab.zig

@@ -416,15 +416,18 @@ pub const tab = struct {
     /// portfolio's memory; invalidate before the underlying data
     /// is freed.
     ///
-    /// Eagerly recomputes only if this tab is active — otherwise
-    /// the next `activate` will lazy-load.
+    /// Deliberately does NOT eager-rebuild, even when this tab
+    /// is active. The reload broadcast fires BEFORE pd.reload
+    /// resets the portfolio arena; rebuilding now would store
+    /// borrowed pointers that get freed seconds later when the
+    /// arena resets, producing use-after-free crashes on the
+    /// next draw. The orchestrator
+    /// (portfolio_tab.reloadPortfolioFile) calls
+    /// `app.loadTabData()` AFTER pd.reload completes, which
+    /// re-activates the active tab against fresh data.
     pub fn onPortfolioReload(state: *State, app: *App) void {
-        if (app.active_tab == .projections) {
-            tab.reload(state, app) catch |err| std.log.debug("projections reload failed: {t}", .{err});
-        } else {
-            freeLoaded(state, app);
-            state.loaded = false;
-        }
+        freeLoaded(state, app);
+        state.loaded = false;
     }
 };
 

src/tui/review_tab.zig

@@ -338,8 +338,16 @@ pub const tab = struct {
     /// edited `acknowledgments.srf` while the TUI was running.
     /// (Same rationale as the user-requested `reload` action.)
     ///
-    /// Eagerly recomputes only when this tab is active —
-    /// otherwise next `activate` lazy-loads.
+    /// Deliberately does NOT eager-rebuild, even when this tab
+    /// is active. The reload broadcast fires BEFORE pd.reload
+    /// resets the portfolio arena; rebuilding now would store
+    /// borrowed pointers (e.g. ReviewRow.bucket borrowing from
+    /// ClassificationEntry.bucket) that get freed seconds later
+    /// when the arena resets, producing use-after-free crashes
+    /// or visual corruption on the next draw. The orchestrator
+    /// (portfolio_tab.reloadPortfolioFile) calls
+    /// `app.loadTabData()` AFTER pd.reload completes, which
+    /// re-activates the active tab against fresh data.
     pub fn onPortfolioReload(state: *State, app: *App) void {
         if (state.view) |*v| v.deinit(app.allocator);
         state.view = null;
@@ -350,9 +358,6 @@ pub const tab = struct {
         state.loaded = false;
         state.cursor = 0;
         state.expanded_finding = null;
-        if (app.active_tab == .review) {
-            tab.activate(state, app) catch |err| std.log.debug("review activate failed: {t}", .{err});
-        }
     }
 
     /// Mouse handling: left-click on the column-header row sorts