From a875fc3b3297acf7d5af021b4f2b1357dde558c0 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 24 Jun 2026 10:24:00 -0700 Subject: [PATCH] fix memory leak in earnings tab --- src/cache/store.zig | 29 ++++++++++++++++++++++++++ src/models/earnings.zig | 44 ++++++++++++++++++++++++++++++++++++++++ src/providers/fmp.zig | 36 +++++++++++++++++++++----------- src/service.zig | 2 +- src/tui/earnings_tab.zig | 6 +++--- 5 files changed, 101 insertions(+), 16 deletions(-) diff --git a/src/cache/store.zig b/src/cache/store.zig index 458aaa9..f67fa07 100644 --- a/src/cache/store.zig +++ b/src/cache/store.zig @@ -1955,6 +1955,35 @@ test "dividend serialize/deserialize round-trip" { try std.testing.expectEqual(DividendType.special, parsed[1].type); } +test "earnings serialize/deserialize round-trip frees duped symbols" { + // Regression test for the earnings-tab memory leak: the cache-read + // path dupes each event's `symbol` string into the caller's + // allocator (string-bearing type, so `parse_allocator = .custom`). + // Freeing only the outer slice leaks every symbol. `freeSlice` must + // release them; testing.allocator fails the test on any leak. + const io = std.testing.io; + const allocator = std.testing.allocator; + const events = [_]EarningsEvent{ + .{ .symbol = "AAPL", .date = Date.fromYmd(2025, 1, 30), .estimate = 2.67, .actual = 2.85 }, + .{ .symbol = "AAPL", .date = Date.fromYmd(2024, 10, 31), .estimate = 1.55, .actual = 1.64 }, + }; + + const data = try Store.serializeWithMeta(EarningsEvent, io, allocator, &events, .{}); + defer allocator.free(data); + + const result = Store.readSlice(EarningsEvent, io, allocator, data, null, .any) orelse return error.TestUnexpectedResult; + // The fix: route the free through freeSlice so the duped symbols go + // too. Before the fix this was `allocator.free(result.data)` and the + // symbols leaked. + defer EarningsEvent.freeSlice(allocator, result.data); + + try std.testing.expectEqual(@as(usize, 2), result.data.len); + // Symbols survived the iterator teardown (proves they were duped). + try std.testing.expectEqualStrings("AAPL", result.data[0].symbol); + try std.testing.expectEqualStrings("AAPL", result.data[1].symbol); + try std.testing.expect(result.data[0].date.eql(Date.fromYmd(2025, 1, 30))); +} + test "split serialize/deserialize round-trip" { const io = std.testing.io; const allocator = std.testing.allocator; diff --git a/src/models/earnings.zig b/src/models/earnings.zig index 1da77e4..7400c4f 100644 --- a/src/models/earnings.zig +++ b/src/models/earnings.zig @@ -48,6 +48,27 @@ pub const EarningsEvent = struct { if (est == 0) return null; return ((act - est) / @abs(est)) * 100.0; } + + /// Free any owned string fields. + /// + /// `symbol` is heap-allocated by both producer paths: the cache-read + /// path dupes it into the caller's allocator (SRF `parse_allocator`, + /// see `cache/store.zig`), and the FMP provider path dupes it in + /// `parseEarningsResponse`. The `len > 0` guard skips the empty-string + /// default used for in-memory/test construction, which points at a + /// string literal rather than the heap. + pub fn deinit(self: EarningsEvent, allocator: std.mem.Allocator) void { + if (self.symbol.len > 0) allocator.free(self.symbol); + } + + /// Free a slice of events, calling `deinit` on each element first. + /// Mirror of `Dividend.freeSlice` — this is what makes + /// `FetchResult(EarningsEvent).deinit()` release the per-event + /// `symbol` strings instead of just the outer slice. + pub fn freeSlice(allocator: std.mem.Allocator, events: []const EarningsEvent) void { + for (events) |e| e.deinit(allocator); + allocator.free(events); + } }; const std = @import("std"); @@ -88,3 +109,26 @@ test "surprisePct" { const miss = EarningsEvent{ .symbol = "AAPL", .date = Date{ .days = 19000 }, .actual = 1.35, .estimate = 1.50 }; try std.testing.expect(miss.surprisePct().? < 0); } + +test "freeSlice releases each event's owned symbol" { + // Mirrors the cache-read / provider path: each event's `symbol` is + // heap-allocated and owned by the slice's allocator. freeSlice must + // release every symbol plus the outer slice. testing.allocator fails + // the test on any leak or double-free. + const allocator = std.testing.allocator; + const events = try allocator.alloc(EarningsEvent, 2); + events[0] = .{ .symbol = try allocator.dupe(u8, "AAPL"), .date = Date{ .days = 19000 } }; + events[1] = .{ .symbol = try allocator.dupe(u8, "MSFT"), .date = Date{ .days = 19001 } }; + EarningsEvent.freeSlice(allocator, events); +} + +test "deinit tolerates the empty-string default (literal, not heap)" { + // An event built with the default symbol points at a string literal, + // not the heap. deinit must not attempt to free it. + const ev = EarningsEvent{ .date = Date{ .days = 19000 } }; + ev.deinit(std.testing.allocator); +} + +test "freeSlice on an empty slice is a no-op" { + EarningsEvent.freeSlice(std.testing.allocator, &.{}); +} diff --git a/src/providers/fmp.zig b/src/providers/fmp.zig index 86ebec0..1b046fa 100644 --- a/src/providers/fmp.zig +++ b/src/providers/fmp.zig @@ -104,7 +104,10 @@ fn parseEarningsResponse( }; var events: std.ArrayList(EarningsEvent) = .empty; - errdefer events.deinit(allocator); + errdefer { + for (events.items) |e| e.deinit(allocator); + events.deinit(allocator); + } for (items) |item| { const obj = switch (item) { @@ -139,8 +142,14 @@ fn parseEarningsResponse( const period_anchor = date.subtractMonths(1); const quarter: u8 = @intCast(((period_anchor.month() - 1) / 3) + 1); - try events.append(allocator, .{ - .symbol = symbol, + // Each event owns its own copy of `symbol`. The cache-read path + // dupes string fields into the caller's allocator, so the provider + // path must too — otherwise the returned slice has mixed ownership + // and no single `freeSlice` can release it correctly. See + // `EarningsEvent.deinit`. + const owned_symbol = try allocator.dupe(u8, symbol); + events.append(allocator, .{ + .symbol = owned_symbol, .date = date, .estimate = estimate, .actual = actual, @@ -152,7 +161,10 @@ fn parseEarningsResponse( .revenue_estimate = optFloat(obj.get("revenueEstimated")), // FMP's /stable/earnings endpoint doesn't expose BMO/AMC timing. .report_time = .unknown, - }); + }) catch |err| { + allocator.free(owned_symbol); + return err; + }; } // Newest-first, matching the UI's expectation. @@ -181,7 +193,7 @@ test "parseEarningsResponse: typical response (AMZN-style)" { ; const events = try parseEarningsResponse(testing.allocator, body, "AMZN"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 3), events.len); @@ -216,7 +228,7 @@ test "parseEarningsResponse: AAPL fiscal-year company quarter mapping" { ; const events = try parseEarningsResponse(testing.allocator, body, "AAPL"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 1), events.len); try testing.expectEqual(@as(?u8, 4), events[0].quarter); @@ -234,7 +246,7 @@ test "parseEarningsResponse: skips empty stub records" { ; const events = try parseEarningsResponse(testing.allocator, body, "SPY"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 0), events.len); } @@ -247,7 +259,7 @@ test "parseEarningsResponse: keeps records with only estimate (upcoming)" { \\] ; const events = try parseEarningsResponse(testing.allocator, body, "X"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 1), events.len); try testing.expect(events[0].actual == null); @@ -264,7 +276,7 @@ test "parseEarningsResponse: keeps records with only actual (no estimate)" { \\] ; const events = try parseEarningsResponse(testing.allocator, body, "AAPL"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 1), events.len); try testing.expectApproxEqAbs(@as(f64, 0.00161), events[0].actual.?, 1e-6); @@ -279,13 +291,13 @@ test "parseEarningsResponse: error envelope returns empty" { \\{"Error Message": "API limit reached"} ; const events = try parseEarningsResponse(testing.allocator, body, "X"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 0), events.len); } test "parseEarningsResponse: empty array" { const events = try parseEarningsResponse(testing.allocator, "[]", "X"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 0), events.len); } @@ -302,7 +314,7 @@ test "parseEarningsResponse: surprise_percent handles zero estimate" { \\] ; const events = try parseEarningsResponse(testing.allocator, body, "X"); - defer testing.allocator.free(events); + defer EarningsEvent.freeSlice(testing.allocator, events); try testing.expectEqual(@as(usize, 1), events.len); try testing.expectApproxEqAbs(@as(f64, 0.05), events[0].surprise.?, 0.001); diff --git a/src/service.zig b/src/service.zig index f7422f5..d8df5e6 100644 --- a/src/service.zig +++ b/src/service.zig @@ -939,7 +939,7 @@ pub const DataService = struct { return .{ .data = cached.data, .source = .cached, .timestamp = cached.timestamp, .allocator = self.allocator }; } // Stale: free cached events and re-fetch below - self.allocator.free(cached.data); + EarningsEvent.freeSlice(self.allocator, cached.data); } } diff --git a/src/tui/earnings_tab.zig b/src/tui/earnings_tab.zig index 5c77d0b..0cb640e 100644 --- a/src/tui/earnings_tab.zig +++ b/src/tui/earnings_tab.zig @@ -65,7 +65,7 @@ pub const tab = struct { /// One-time teardown. Free any allocated payloads. pub fn deinit(state: *State, app: *App) void { - if (state.data) |e| app.allocator.free(e); + if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e); state.* = .{}; } @@ -92,7 +92,7 @@ pub const tab = struct { } // Clear every flag so loadData has the same starting // conditions as a fresh activation. - if (state.data) |e| app.allocator.free(e); + if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e); state.* = .{}; loadData(state, app); } @@ -111,7 +111,7 @@ pub const tab = struct { /// next `activate` re-fetches for the new symbol. Distinct /// from `reload` (no fetch is triggered here). pub fn onSymbolChange(state: *State, app: *App) void { - if (state.data) |e| app.allocator.free(e); + if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e); state.* = .{}; }