Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/app.zig
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,15 @@ pub const App = struct {
// Flow Runtime frees its subscribed-flow name copies.
self.entity_inspector.deinit();
self.flow_runtime.deinit(self.allocator);
// Revert the process-global flow-node catalog to its static
// fallback, freeing the project's runtime catalog (and its arena)
// if one was installed by `reloadFlowNodeCatalog`. Without this,
// the last-loaded `RuntimeCatalog` outlives the App: `setRuntime`
// only frees the *previous* catalog on the next project
// transition, and shutdown is not a transition — so the catalog's
// arena (the materialized `entries`/`pin_styles` slices grown from
// ArrayLists in `loadFromPath`) leaks on exit (labelle-gui#209).
flow_node_catalog.setRuntime(null);
self.project_manager.deinit();
self.tree_view.deinit();
self.compiler.deinit();
Expand Down
64 changes: 64 additions & 0 deletions src/flow_node_catalog.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,70 @@ test "setRuntime: swap to runtime, then revert to static" {
try std.testing.expectEqual(static_count, entries.len);
}

// Regression for labelle-gui#209: a runtime catalog installed by a
// project load (and never followed by another project transition) must
// be reclaimed at App shutdown. `App.deinit` now does this via a final
// `setRuntime(null)`. This test mirrors that lifecycle under
// `testing.allocator` — load a real sidecar, install it, then revert
// exactly once (as shutdown does). If the final revert is dropped, the
// catalog's arena (holding the ArrayList-grown `entries`/`pin_styles`
// slices) leaks and `testing.allocator` fails the test — which is the
// exact DebugAllocator leak the issue reported on window close.
test "setRuntime: shutdown revert frees a sidecar-loaded catalog (issue #209)" {
const aa = std.testing.allocator;
var tmp = std.testing.tmpDir(.{});
defer tmp.cleanup();

const json =
\\{
\\ "generated_at": "2026-06-09T00:00:00Z",
\\ "plugins": [
\\ {
\\ "name": "synthetic",
\\ "flow_nodes": [
\\ {
\\ "qualified": "synthetic.do_thing",
\\ "display_name": "Do Thing",
\\ "category": "synthetic",
\\ "docs": "",
\\ "kind": "command",
\\ "pins": [
\\ { "name": "entity", "label": "Entity", "zig_type": "u32", "dir": "input", "default": null }
\\ ],
\\ "return_type": null
\\ }
\\ ],
\\ "pin_styles": [
\\ { "zig_type": "MyStruct", "label": "My Struct", "color": [12, 34, 56] }
\\ ]
\\ }
\\ ]
\\}
\\
;
try tmp.dir.writeFile(io_global.io(), .{ .sub_path = "flow_catalog.json", .data = json });
const path = try tmp.dir.realPathFileAlloc(io_global.io(), "flow_catalog.json", aa);
defer aa.free(path);

// Project-load path: parse the sidecar and install it as the active
// runtime catalog (what `App.reloadFlowNodeCatalog` does).
const cat = try loadFromPath(aa, path);
setRuntime(cat);
// Guarantee cleanup even if an assertion below fails — otherwise an
// early exit leaks `cat` and leaves `current_runtime` dirty for the
// next test. `setRuntime` is idempotent, so this is a no-op after the
// explicit revert succeeds (gemini #211).
defer setRuntime(null);
try std.testing.expect(current_runtime != null);
try std.testing.expectEqualStrings("synthetic.do_thing", entries[0].name);

// Shutdown path: the single revert `App.deinit` now performs. This
// is the *only* free of `cat`'s arena — drop it and the test leaks.
setRuntime(null);
try std.testing.expect(current_runtime == null);
try std.testing.expectEqual(static_pin_styles.len, pin_styles.len);
}
Comment on lines +1199 to +1213

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If any of the assertions fail before setRuntime(null) is reached, the test will exit early, leaking the allocated RuntimeCatalog and leaving the global current_runtime state dirty for subsequent tests.

Since setRuntime(null) is idempotent and safe to call multiple times, we can use defer setRuntime(null); immediately after setRuntime(cat); to guarantee cleanup on failure.

    setRuntime(cat);
    defer setRuntime(null);
    try std.testing.expect(current_runtime != null);
    try std.testing.expectEqualStrings("synthetic.do_thing", entries[0].name);

    // Shutdown path: the single revert App.deinit now performs. This
    // is the *only* free of cat's arena — drop it and the test leaks.
    setRuntime(null);
    try std.testing.expect(current_runtime == null);
    try std.testing.expectEqual(static_pin_styles.len, pin_styles.len);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — defer setRuntime(null); right after install, so a failed assertion can't leak cat or leave current_runtime dirty. The explicit revert (the thing under test) stays; the defer is an idempotent no-op after it.


test "loadFromSidecar: returns null when no .labelle/* subdir has the file" {
const aa = std.testing.allocator;
var tmp = std.testing.tmpDir(.{});
Expand Down
Loading