fix: free the runtime flow-node catalog on App.deinit (shutdown leak) (#209)#211
Conversation
The editor leaked an allocation on exit whenever a project with a .labelle/flow_catalog.json sidecar was loaded. App.reloadFlowNodeCatalog installs a RuntimeCatalog into the process-global current_runtime via setRuntime(cat); setRuntime only frees the *previous* catalog on the next project transition. Shutdown is not a transition, so the last-loaded catalog's arena -- holding the entries/pin_styles slices grown from ArrayLists in loadFromPath -- outlived App.deinit and was reported as a leaked ArrayList grow by the DebugAllocator on window close. Fix: App.deinit now calls flow_node_catalog.setRuntime(null), reverting to the static fallback and freeing the active runtime catalog. Adds a leak-checked regression test that loads a real sidecar, installs it as the active catalog (the project-load path), and reverts exactly once (the shutdown path). Verified the test reproduces the leak: with the revert skipped it reports 'Memory Leak Detected' under testing.allocator. zig build clean; zig build test 464/464; zig build gui-test 26/26.
PR SummaryLow Risk Overview
A regression test in Reviewed by Cursor Bugbot for commit b2ee3cd. Bugbot is set up for automated code reviews on this repo. Configure here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a memory leak (issue #209) by ensuring that the process-global flow-node catalog is reverted to its static fallback via setRuntime(null) during application shutdown in App.deinit. It also adds a regression test to verify this behavior. The review feedback suggests using defer setRuntime(null); in the test to guarantee cleanup and prevent global state pollution if any assertions fail before the manual cleanup is reached.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| setRuntime(cat); | ||
| 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); | ||
| } |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Fixes a shutdown-time memory leak by ensuring the process-global runtime flow-node catalog is freed when the app exits (not only on project transitions).
Changes:
- Call
flow_node_catalog.setRuntime(null)fromApp.deinitto free the active runtime catalog arena on shutdown. - Add a regression test that installs a sidecar-loaded runtime catalog and verifies a single shutdown-style revert restores the static catalog without leaking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/flow_node_catalog.zig |
Adds a regression test covering shutdown-style cleanup of a sidecar-loaded runtime catalog. |
src/app.zig |
Ensures shutdown resets the global flow-node catalog to the static fallback, freeing the last runtime catalog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const cat = try loadFromPath(aa, path); | ||
| setRuntime(cat); | ||
| try std.testing.expect(current_runtime != null); | ||
| try std.testing.expectEqualStrings("synthetic.do_thing", entries[0].name); | ||
|
|
Closes #209.
Root cause
The process-global
current_runtime: ?*RuntimeCatalog(src/flow_node_catalog.zig) holds an arena with theentries/pin_stylesslices built viaArrayList.append+toOwnedSlice(theensureTotalCapacityPrecisegrow the #209 trace showed).App.reloadFlowNodeCataloginstalls a catalog viasetRuntime(cat)when a project has a.labelle/flow_catalog.jsonsidecar (bouncing-ball does), butsetRuntimeonly frees the PREVIOUS catalog on the next project transition. Shutdown isn't a transition, so the last-loaded catalog's arena leaked on window close.The
sendFile WriteFailedin the trace is a red herring — it's the genericerror.WriteFailedfromstd.Io.Dir.writeFile(any failed save), on a separate path from the catalog (parsed on project LOAD). The leak is unconditional after loading a sidecar project, not gated on a failed write.Fix
App.deinitnow callsflow_node_catalog.setRuntime(null), reverting to the static fallback and freeing the active runtime catalog + arena.Verification — reproduced under testing.allocator
Regression test in
flow_node_catalog.zig: loads a real sidecar, installs it (project-load path), reverts once (shutdown path) understd.testing.allocator. Confirmed it catches the leak — skipping the free makes it report "Memory Leak Detected"; with the fix it passes. (Existing gui-tests missed it: they open.flow.jsoncwithout a sidecar-bearing project, andgui_tests.zigdiscards the DebugAllocator result.)zig build test464/464;zig build gui-test26/26;zig buildclean.