prolly: plug remaining TableEntry zName leaks across call sites#464
Merged
prolly: plug remaining TableEntry zName leaks across call sites#464
Conversation
5b7a250 to
7f64c1a
Compare
Follow-up to #461, which introduced doltliteFreeCatalog and fixed the dolt_add -A path. Applying the helper across the rest of the doltliteLoadCatalog call sites that had the same class of leak: free the array backbone but not the per-entry zName strings. Two shapes of caller: 1) Read-only consumers that just walk the returned array and free it at the end. Mechanical replacement of sqlite3_free(aX) with doltliteFreeCatalog(aX, nX): - doltlite_at.c - doltlite_history.c - doltlite_ref.c (all three frees) - doltlite_status.c (statusFilter: aHead/aStaged/aWorking) - doltlite_diff.c (collectSummaryForWorking + collectSummaryForCommit) - doltlite_diff_table.c (loadTblRootAtCommit) - doltlite_diff_stat.c (dsComputeTableStats, dsCollectTableNames, dssFilter) - doltlite_schema_diff.c (loadSchemaFromCatalog + sdFilter) - doltlite_schema_merge.c (applySchemaMergeActions) - doltlite_blame.c (loadTableRoot) - doltlite_dbpage.c (dbpage page-1 header synth) 2) Shallow-copy consumers that move TableEntry values between arrays (the source transfers zName ownership non-explicitly). Rewritten as dup-on-copy so each array has independent ownership and can be freed with doltliteFreeCatalog: - doltlite_branch.c (checkout-paths branch: aWorking <- aSource append, in-place overwrite, memmove-delete) - doltlite.c doltliteAddFunc explicit-name path: aStaged <- aWorking with append, overwrite, and memmove-delete - doltlite.c doltliteCommitFunc addModifiedOnly: aStaged <- aWorking with append, overwrite, and memmove-delete - doltlite.c doltliteResetFunc path branch: aStaged <- aHead with append, overwrite, and memmove-delete - doltlite.c doltliteResetFunc --hard untracked-preserve: aWorking <- aTarget in-place overwrite Deliberately LEFT ALONE: doltlite_merge.c's mergeCatalogPass1 / Pass2 build aMerged with a mix of shallow-copies from aOurs / aTheirs / aAnc (borrowing zName) and fresh sqlite3_mprintf allocations (line 1259 in the i<nTheirs loop). aMerged's mixed ownership makes any free strategy wrong — doltliteFreeCatalog on the sources double-frees the aliased entries, plain free leaks the fresh ones. Proper fix requires restructuring pass1/pass2 to always dup into aMerged; that's a bigger surgery and is left for a follow-up. First attempt at treating merge.c the same way as the others produced SIGABRT under doltlite_reset.sh's merge conflict scenario, confirming the hazard. Full regression green: 39/39 doltlite suites, 16/16 add oracle, 11/11 status oracle, 16/16 branch oracle, 15/15 reset oracle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7f64c1a to
52dfe5b
Compare
Sysbench-Style Benchmark: Doltlite vs SQLiteIn-MemoryReads
Writes
File-BackedReads
Writes
10000 rows, single CLI invocation per test, workload-only timing via SQL timestamps. Performance Ceiling Check (3x)All tests within 3x ceiling. |
timsehn
added a commit
that referenced
this pull request
Apr 15, 2026
detect_leaks=0 was set because the stock sqlite3 shell trips the short-lived-leak detector on process exit — not a doltlite bug, but loud enough to hide real leaks in our CI signal. Now that #461 and #464 plugged the TableEntry zName leaks across every catalog caller, we want that signal back so the next leak gets caught at PR time. Enable detect_leaks=1 and wire a narrow LSAN_OPTIONS suppressions file at .github/lsan.supp that filters only the specific stock sqlite allocations that leak by design (shell input buffer, builtin test extensions). Every other allocation gets full coverage — if CI flags a leak, it's either in doltlite or in a path we own. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 task
timsehn
added a commit
that referenced
this pull request
Apr 15, 2026
detect_leaks=0 was set because the stock sqlite3 shell trips the short-lived-leak detector on process exit — not a doltlite bug, but loud enough to hide real leaks in our CI signal. Now that #461 and #464 plugged the TableEntry zName leaks across every catalog caller, we want that signal back so the next leak gets caught at PR time. Enable detect_leaks=1 and wire a narrow LSAN_OPTIONS suppressions file at .github/lsan.supp that filters only the specific stock sqlite allocations that leak by design (shell input buffer, builtin test extensions). Every other allocation gets full coverage — if CI flags a leak, it's either in doltlite or in a path we own. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #461. Applies
doltliteFreeCatalogto the remaining call sites ofdoltliteLoadCatalogthat had the same "free the array backbone but not the per-entry zName" leak.Two shapes of caller
(A) Read-only consumers — just walk the returned array and free at the end. Mechanical replacement of
sqlite3_free(aX)withdoltliteFreeCatalog(aX, nX):doltlite_at.c,doltlite_history.c,doltlite_ref.cdoltlite_status.c(statusFilter)doltlite_diff.c(collectSummaryForWorking+collectSummaryForCommit)doltlite_diff_table.c(loadTblRootAtCommit)doltlite_diff_stat.c(dsComputeTableStats,dsCollectTableNames,dssFilter)doltlite_schema_diff.c,doltlite_schema_merge.cdoltlite_blame.c,doltlite_dbpage.c(B) Shallow-copy consumers — move
TableEntryvalues between arrays (the source transfers zName ownership non-explicitly). Rewritten as dup-on-copy so each array has independent ownership and can be freed withdoltliteFreeCatalog:doltlite_branch.ccheckout-paths branch (aWorking←aSourceappend / overwrite / memmove-delete)doltlite.cdoltliteAddFuncexplicit-name path (aStaged←aWorking)doltlite.cdoltliteCommitFuncaddModifiedOnlypath (aStaged←aWorking)doltlite.cdoltliteResetFuncpath-branch (aStaged←aHead)doltlite.cdoltliteResetFunc--harduntracked-preserve (aWorking←aTargetin-place overwrite)Deliberately left alone
doltlite_merge.c'smergeCatalogPass1/Pass2buildaMergedwith a mix of shallow-copies fromaOurs/aTheirs/aAnc(borrowing zName) and freshsqlite3_mprintfallocations at line 1259 in thei<nTheirsloop. Mixed ownership makes any naive free strategy wrong:doltliteFreeCatalogon the sources double-frees the aliased entriessqlite3_freeonaMergedleaks the fresh onesProper fix requires restructuring pass1/pass2 to always dup into aMerged — that's a bigger surgery and is left for a follow-up. First attempt at treating
merge.cthe same way as the others producedSIGABRTunderdoltlite_reset.sh's merge-conflict scenario, confirming the hazard. Reverted the merge change for this PR.Test plan
make doltlite— cleanbash test/run_doltlite_tests.sh— 39/39 suitesbash test/vc_oracle_add_test.sh— 16/16bash test/vc_oracle_status_test.sh— 11/11bash test/vc_oracle_branch_test.sh— 16/16bash test/vc_oracle_reset_test.sh— 15/15🤖 Generated with Claude Code