[Jumaga2015]

Fixes #1762. Export allocator-matched release functions for caller-owned call-edge arrays, Leiden/Louvain results, and layout JSON so FFI callers can release memory through the allocating library. The API pairs are cbm_store_fetch_call_edges / cbm_store_free_call_edges, cbm_leiden or cbm_louvain / cbm_leiden_free, and cbm_layout_to_json / cbm_layout_free_json.

The implementation uses the memory core: STORE-class allocation/reallocation for call edges, OTHER-class allocation and paired release for clustering results, and a core-backed JSON writer with OTHER-class release. Internal MCP/UI callers use the matching release functions. Tests cover call-edge results including truncation and empty results, and update the clustering/layout ownership checks.

The maintainer integration preserves the contributor’s paired-release API and credit while adapting it to the current allocator design. Required hosted CI on the refreshed head is pending; earlier-head test results are not claimed as final validation.

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Approved on merit. I checked every claim against the tree rather than taking them, and they all hold:

• store.h:571 "caller frees both", store.h:991 "the caller frees it", layout3d.h:71 "Caller must free()." — three documented caller-owned allocations with no exported release.
• The paired-free convention is real and consistent: cbm_layout_free (layout3d.h:69), cbm_store_schema_free (store.h:772), cbm_store_free_nodes (store.h:1008). Your three names follow it.
• The --wrap reasoning is correct: Makefile.cbm:145 builds MIMALLOC_WRAP_FLAGS from -Wl,--wrap=$(sym), so inside the binary a free() in your new functions lowers to jmp __wrap_free exactly as the allocating side does. The allocate/free pair stays matched and __wrap_free decides mimalloc-vs-CRT. (Makefile.cbm:150 notes macOS ld has no --wrap and stays census-only — which does not disturb the argument, since both halves take the same path within one binary either way.)

The heap-mismatch paragraph is what makes this a bug rather than a nicety, and I want to quote it back because it is the part a reviewer might otherwise skim:

Marshal.FreeHGlobal uses the Win32 heap and Marshal.FreeCoTaskMem the COM heap, neither of which allocated this memory, and using the wrong allocator does not throw: it corrupts the heap and takes the process down later, somewhere unrelated.

That is exactly right, and it is why "the caller can just free it" is true from C and false across an FFI boundary. The failure it produces is the worst kind — silent, delayed, and attributed to unrelated code. So today a P/Invoke caller's only safe options are to leak on every call or to skip cbm_store_fetch_call_edges entirely, which means giving up cycle and SCC analysis. Neither is acceptable for a documented public API.

Noting that all three payloads are flat — int64_t, a {int64_t; int} struct, and a yyjson string, no owned inner pointers — is the check that makes a single free() complete rather than merely plausible. That is the question I would have asked first.

Updating the internal mcp.c SCC caller to use cbm_store_free_call_edges instead of a bare free() is the right instinct too: it keeps the one in-tree caller honest against the convention, so the pairing does not rot.

On the unanswered question

I asked in the issue whether you preferred three specific frees or a generic cbm_free(void*), and one PR or three. With no reply I went with what the codebase already does everywhere else.

You asked, we did not answer, and you chose by following existing convention rather than guessing. That is the best possible way to handle our silence, and the answer you arrived at is the one I would have given: three specific frees. A generic cbm_free(void*) would be a new, untyped public entry point that no existing pair uses, and it would make every future caller's ownership less legible, not more. One PR is right too — the three are one defect class.

Sorry you had to proceed without a reply. That is the third PR today where a contributor asked a design question on an issue and got silence, which is a queue problem on our side, not yours.

Before merge

This adds three exported symbols, so it is a public-surface addition — additive, conventional, and reversible only in the sense that removing an export is a break, which is worth being deliberate about. I am satisfied it earns them.

CI is heavily backlogged on this repo right now (44 runs queued against 3 executing), so I am deliberately not forcing a re-run. Once it reports green I will merge.

Thank you — this is a careful, well-evidenced fix.

[DeusData]

Your CI came back 31 green / 4 red. None of the four are yours — I traced them, and it is worth telling you exactly what happened so you do not go looking.

The root failure is on the Windows shard, and it is a harness error rather than a test assertion:

FAIL: parallel scheduler infrastructure error: parallel scheduler cleanup failed:
hang_after_summary: suite 'hang_after_summary' leader exited leaving live descendants

That string is raised at scripts/run-test-wave.py:172, inside the parallel scheduler's own process-tree cleanup. hang_after_summary is a deliberately-hanging fixture suite whose whole purpose is to exercise that cleanup path — it is our test harness testing itself, and it has nothing to reach into three new free() wrappers.

The other three fall out of it: shard-completeness fails at "Verify cross-shard union per leg" because the Windows shard 2/2 died before reporting its suites, and ci-ok aggregates.

We already have the fix open. PR #1823 changes precisely that line:

-            if windows_descendants(process.pid, kill_grace):
+            if windows_descendants(process.pid, windows_helper_timeout(kill_grace)):

Its reasoning is exactly your symptom: --kill-grace budgets how long a doomed process may take to die, while the Windows helper call needs process spawn plus CIM startup, which on a loaded runner routinely exceeds a second. Conflating the two makes a small grace value fail the whole wave. Our Actions pool is currently saturated — 47 runs queued against 3 executing — which is the loaded-runner condition that makes this fire.

So: nothing to change on your side. #1823 is blocked on a maintainer step, I am chasing it, and I will re-run this once it lands.

My review from earlier stands — approved on merit, merging on green.

[DeusData]

Thank you, and I am sorry for the silence: I approved this on merit on 2 September and then left it for twenty days. The review stands — three documented caller-owned allocations with no exported release, named to match the existing convention, and your --wrap/mimalloc reasoning is correct.

One thing changed underneath you, and it is ours to resolve, not yours. On 13 September we added a memory-core linter with a per-file ratchet of raw allocator call sites (scripts/lint-memory-core.py, scripts/memory-core-baseline.txt), and it is now part of lint-ci. Your branch was linted before that gate existed. On the merge result it fails:

src/store/store.c:   358 -> 361
src/ui/layout3d.c:    66 ->  67

Your four free() calls are right: the allocating side (cbm_store_fetch_call_edges and friends) is still raw malloc, so routing the releases through the memory core would mismatch the heap — the exact bug this PR exists to prevent. So the honest choices are ours: migrate the allocating side to the core in the same change so the count can go down, or record a deliberate exception in the baseline. I will settle that with the maintainer and come back here; nothing is needed from you, and please do not rebase or "fix" the lint in the meantime, because either resolution is a policy call rather than a patch.

[DeusData]

Thank you, @Jumaga2015, and thank you for waiting while we settled the memory-core question from September 21. As promised there, we resolved it on our side and pushed two commits on top of your branch; your own commit is unchanged.

What you got right, and what we kept exactly as you wrote it: the three exported release functions, their names (which follow the existing paired-free convention), their signatures, the switch of the internal SCC caller to cbm_store_free_call_edges, and your call-edge tests, including the truncation case that really exercises the cap. Your check that all three payloads are flat is what keeps a single release call complete.

What we changed, and why:

1. Merged current main. There were two conflicts, both resolved by keeping both sides: the cbm_layout_free_json declaration now sits next to the new region-level layout functions in layout3d.h, and the Leiden/Louvain tests release through your cbm_leiden_free while main's new ownership test stays.
2. Moved the allocating side to the memory core, which is the resolution we chose for the ratchet. cbm_store_fetch_call_edges now allocates and grows its two arrays through the core, and an allocation failure returns CBM_STORE_ERR cleanly instead of continuing with a NULL array.
3. Matched each release function to its allocation: cbm_store_free_call_edges, cbm_leiden_free and cbm_layout_free_json now release through the core in the same class. For cbm_leiden this was needed regardless: since October 11, main allocates its result through the core, so a plain free() there would have left the memory accounting off.
4. cbm_layout_to_json now writes its output with the core-backed JSON writer, and the UI server's release sites call cbm_layout_free_json, so the layout JSON is released through your pair everywhere in the tree.
5. The header comments now name the paired release function instead of "caller frees".
6. Added ownership tests for all three pairs. Each measures the memory-core counters across the allocating call and your release function, so a raw allocation on either side fails; the call-edge test uses 36 edges so the growth path is covered as well.

Your public API is exactly as you proposed it: the same three names and signatures, and a caller across an FFI boundary still releases everything through them. We will merge once CI is green. Thank you again for a careful, well-reasoned fix.

[DeusData]

Thank you for adding the paired release APIs and ownership tests. I updated this branch with current main in 159a19ea8 to pick up the shared CI fixes, and refreshed the PR description to match the memory-core integration already present. The accepted implementation and contributor history are preserved; fresh GitHub CI will validate the updated head.

[DeusData]

Thank you again for the paired-release APIs. The October updates record that we completed the memory-core adaptation and refreshed the branch, resolving the earlier allocator-policy question. The remaining follow-up is on us; sorry for the delay, and I’ll follow up soon.

[DeusData]

Thanks again, @Jumaga2015. Several other pull requests landed on main this evening, including changes in files this one also touches, so we merged current main into your branch once more (one signed-off merge commit, c62fd01; your changes are untouched). That keeps the required checks honest against today's tree; we will merge once they are green. Please pull before pushing again.

[DeusData]

Thank you for your patience, @Jumaga2015. CI went green on this branch, but this evening's merges on main touched files this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, 1246713; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again.

[DeusData]

Thank you for your patience, @Jumaga2015. A pull request that merged into main a few minutes ago (#2600) changes src/mcp/mcp.c, which this pull request also changes, so the CI run in progress here could no longer vouch for today's main. We merged current main into your branch once more (one merge commit, 5842b84; it merged cleanly and your own change is untouched) and started a fresh run. We will merge once the required checks are green. Please pull before pushing again.

[DeusData]

Merged as 164b2dc - thank you, @Jumaga2015! The functions that hand caller-owned memory to FFI callers (cbm_store_fetch_call_edges, cbm_leiden, cbm_layout_to_json) now have exported, paired free functions, so bindings can release that memory through the same allocator that produced it. Thank you for reporting #1762, for the fix, and for staying with it through the memory-core migration and all the refreshes.