Fix the per-query memory leak in the C accelerator by kesmit13 · Pull Request #136 · singlestore-labs/singlestoredb-python

kesmit13 · 2026-09-28T15:01:33Z

Fixes #135.

_PyUnicode_AsUTF8 in accel.c took a new reference to the encoded
bytes and never dropped it, so every query leaked one block per column
plus one for the encoding errors string. Measured on a 200-column query,
3000 iterations: ~50 MB of RSS growth, and 201 blocks per query —
matching the numbers in the issue.

There were four call sites, not the three the issue lists;
get_numpy_col_type leaks the same bytes (it does free its C copy).

What changed

The helper now Py_DECREFs the bytes on every path, and checks
calloc before writing through it. Py_LIMITED_API stays at 3.8, so
the hand-rolled copy stays as well — PyUnicode_AsUTF8AndSize is not
available to us until the floor moves to 3.10.
Per-column encodings were leaking their C copies too:
State_clear_fields freed the array and not the entries in it.
Struct sequence field names could not be freed the same way.
PyStructSequence_NewType stores the name pointers rather than copying
the strings (initialize_members in Objects/structseq.c), and
structseq_repr reads them again later. Rows can outlive the State
that built them, so freeing the names in State_clear_fields would
have traded the leak for a use-after-free in repr(row). They now
belong to a capsule in the type's own dict, freed when the last
reference to the type goes away. State_clear_fields only frees them
on the paths where the capsule never took ownership, and now does so
after Py_CLEAR(self->structsequence) rather than before.

Verification

singlestoredb/tests/test_accel_leaks.py runs a 100-column query 200
times per results_type and asserts the sys.getallocatedblocks()
delta stays under 5 blocks/query. On this branch it measures 0.00; on
main it fails at 101.005. Skipped when the extension is unavailable,
under SINGLESTOREDB_PURE_PYTHON=1, and on HTTP connections.

The second test holds struct sequence rows, churns the State, then
reads every field and the repr — it passes on main too (the leak is
what kept those names alive), and exists to stop this fix regressing
into a use-after-free.

Before and after, 3000 iterations:

	`main`	this branch
1 column	2.00 blocks/query	0.00
200 columns	201.00	0.00
200 columns, structsequences	401.00	0.00
maxrss, 200 columns	+50 MB (+101 MB structseq)	0 MB (+2.7 MB structseq, one-shot)

Full non-management suite: 851 passed, 19 skipped.

🤖 Generated with Claude Code

Note

Medium Risk
Changes native extension memory ownership and struct-sequence field lifetime; incorrect freeing could cause use-after-free in row repr, though new tests target that regression.

Overview
Fixes per-query memory leaks in the C MySQL accelerator (accel.c), including the issue #135 pattern of ~one allocated block per result column.

_PyUnicode_AsUTF8 now always releases the intermediate bytes object (previously leaked on every call) and handles calloc failure on the error path. Call sites for column encodings and struct-sequence field names propagate allocation failures instead of leaving bad state.

State_clear_fields frees each per-column encoding C string, not only the pointer array. Struct-sequence field name strings are no longer torn down with query state: they move into a module registry (weak ref to the row type → capsule with destructor) so names stay valid for repr on rows that outlive the fetch state.

Adds test_accel_leaks.py: sys.getallocatedblocks() budgets for per-column and per-query retention across results_type modes, plus struct-sequence lifetime checks after state churn and type-dict stripping.

^{Reviewed by Cursor Bugbot for commit 2a1b3b6. Bugbot is set up for automated code reviews on this repo. Configure here.}

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved lifetime-safety, allocation-error handling, and test configuration issues remain.

Review effort: Lite
Findings: 1 · 1

Open (2)

Mutable type attribute can free field names while rows still reference them · New
Test boolean parsing rejects valid true/yes/on configuration values · New

What changed in this PR

Fixes per-query memory leaks in the C accelerator while preserving struct-sequence row lifetime.

Changes:

Releases temporary UTF-8 and per-column allocations.
Adds ownership handling for struct-sequence field names.
Adds leak and row-lifetime regression tests.

File	Description
`singlestoredb/tests/test_accel_leaks.py`	Adds memory-leak and lifetime regression coverage.
`accel.c`	Updates allocation cleanup and struct-sequence ownership.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Address review feedback on #136. The capsule owning a struct sequence type's field name storage was stored as __singlestoredb_fields__ on the type, where user code could delete or replace it. That decrefs the capsule and frees the names while the type and its existing rows still point at them, so a later repr(row) reads freed memory -- confirmed as a UnicodeDecodeError off garbage bytes. Ownership now lives in a module-private dict inside the extension, keyed by a weak reference to the type. The weakref callback drops the entry, and with it the capsule, when the type is collected, so there is one free path and no Python-reachable way to trigger it early. Rows are instances of a heap type and hold a reference to it, so the type still outlives every row. Also read the already-parsed pure_python option in the tests instead of parsing SINGLESTOREDB_PURE_PYTHON with int(), which raised at collection time for valid values such as `true`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

_PyUnicode_AsUTF8 took a new reference to the encoded bytes and never dropped it, so every query leaked one block per column plus one for the encoding errors string -- around 50 MB per 3000 wide queries, which is enough to OOM a long-running service. Fixes #135. The per-column encodings were leaking their C copies too: State_clear_fields freed the array and not the entries in it. The struct sequence field names cannot be freed the same way. The type stores the name pointers rather than copying the strings, and reads them again when a row is repr'd, so they have to outlive every row rather than the State that built them -- freeing them here would trade the leak for a use-after-free. They now belong to a capsule in the type's own dict, which frees them when the last reference to the type goes away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Address review feedback on #136. The capsule owning a struct sequence type's field name storage was stored as __singlestoredb_fields__ on the type, where user code could delete or replace it. That decrefs the capsule and frees the names while the type and its existing rows still point at them, so a later repr(row) reads freed memory -- confirmed as a UnicodeDecodeError off garbage bytes. Ownership now lives in a module-private dict inside the extension, keyed by a weak reference to the type. The weakref callback drops the entry, and with it the capsule, when the type is collected, so there is one free path and no Python-reachable way to trigger it early. Rows are instances of a heap type and hold a reference to it, so the type still outlives every row. Also read the already-parsed pure_python option in the tests instead of parsing SINGLESTOREDB_PURE_PYTHON with int(), which raised at collection time for valid values such as `true`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The namedtuples subtest of test_no_leak_per_query failed in CI while passing locally, reporting a steady 15.005 retained blocks per query. That is not our allocation. coverage.py's sys.monitoring backend keeps every code object it ever sees alive forever, deliberately, keyed by id() -- see code_objects in coverage/sysmon.py. collections.namedtuple compiles a fresh __new__ on every call and the accelerator builds one Row class per query, so a --cov run, which is what code-check.yml does, retains a code object per query with nothing wrong. pandas' DataFrame.itertuples builds its class the same way with no cache, so this is the tracer's accounting rather than a C API artifact or ours to fix. The leak in issue #135 cost one allocation per column per query, and the accelerator now measures flat at 15.005 blocks per query for both a 10-column and a 100-column result -- a constant that does not move when the width grows tenfold was never that bug. So assert the property the bug actually had: difference a narrow and a wide query, which cancels every per-query cost that is flat in the column count. The tracer overhead is flat, measured unchanged from 5 to 200 columns, so it cancels exactly rather than approximately. Keep a separate absolute per-query check, since differencing two widths cannot see something leaked once per query, and fund the namedtuples budget with an overhead figure measured at runtime so it stays tight when nothing is tracing. Verified both ways: the differential assertion still reports 1.0 blocks per column for tuples, dicts and namedtuples and 2.0 for structsequences when built against accel.c as of 4e348a8, twenty times the threshold, and the whole file passes on 3.11 and on 3.14.7 with and without --cov. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

The per-column encoding caller must handle _PyUnicode_AsUTF8 allocation failure before approval.

Review effort: Lite
Findings: None

Resolved since last review (2)

Mutable type attribute can free field names while rows still reference them
Test boolean parsing rejects valid true/yes/on configuration values

Copilot

Copilot review overview

🟡 Changes recommended

The per-column encoding allocation failure path in accel.c must be handled before approval.

Review effort: Lite
Findings: 1

Open (1)

Check encoding allocation failure before continuing · New

_PyUnicode_AsUTF8 returns a calloc'd copy now, so it can fail. Three of its four call sites check for that; the per-column one in State_init did not, and NULL is the binary-column sentinel every reader of encodings[] goes by (accel.c:1944, 1974, 2159). A failed allocation for a text column would therefore have decoded that column as binary, with the PyErr_NoMemory only surfacing when the function exited. Split the ternary into an explicit branch so the failure is distinguishable from the sentinel and reaches the error label. The !py_encoding arm it replaces was dead -- the PyTuple_GetItem above already bails on NULL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Allocation-failure paths must clear the struct-sequence before releasing its capsule to prevent dangling metadata.

Review effort: Lite
Findings: None

Resolved since last review (1)

Check encoding allocation failure before continuing

kesmit13 requested review from mgiannakopoulos, pmishchenko-ua and volodymyr-memsql as code owners September 28, 2026 15:01

kesmit13 had a problem deploying to Base September 28, 2026 15:01 — with GitHub Actions Error

kesmit13 requested a lite review from Copilot September 28, 2026 19:14

Copilot started reviewing on behalf of kesmit13 September 28, 2026 19:14 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread accel.c Outdated

Comment thread singlestoredb/tests/test_accel_leaks.py Outdated

kesmit13 had a problem deploying to Base September 29, 2026 12:55 — with GitHub Actions Failure

kesmit13 deployed to Base September 29, 2026 13:06 — with GitHub Actions Active

kesmit13 and others added 2 commits September 29, 2026 09:14

kesmit13 force-pushed the fix-accel-utf8-leak branch from c0cccc6 to edee40e Compare September 29, 2026 13:15

kesmit13 had a problem deploying to Base September 29, 2026 13:15 — with GitHub Actions Failure

kesmit13 deployed to Base September 29, 2026 13:35 — with GitHub Actions Active

kesmit13 deployed to Base September 29, 2026 13:47 — with GitHub Actions Active

kesmit13 requested a lite review from Copilot September 29, 2026 13:50

Copilot started reviewing on behalf of kesmit13 September 29, 2026 13:51 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

kesmit13 deployed to Base September 29, 2026 14:56 — with GitHub Actions Active

kesmit13 requested a lite review from Copilot September 29, 2026 16:34

Copilot started reviewing on behalf of kesmit13 September 29, 2026 16:35 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Comment thread accel.c

kesmit13 deployed to Base September 29, 2026 16:47 — with GitHub Actions Active

kesmit13 requested a lite review from Copilot September 29, 2026 16:49

Copilot started reviewing on behalf of kesmit13 September 29, 2026 16:50 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

kesmit13 deployed to Base September 29, 2026 16:56 — with GitHub Actions Active

kesmit13 deployed to Base September 29, 2026 17:54 — with GitHub Actions Active

kesmit13 merged commit b342935 into main Sep 29, 2026
17 checks passed

kesmit13 deleted the fix-accel-utf8-leak branch September 29, 2026 18:34

Navigation Menu

Fix the per-query memory leak in the C accelerator - #136

Fix the per-query memory leak in the C accelerator#136
kesmit13 merged 4 commits into
mainfrom
fix-accel-utf8-leak

kesmit13 commented Sep 28, 2026 •

edited by cursor Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kesmit13 commented Sep 28, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kesmit13 commented Sep 28, 2026 •

edited by cursor Bot

Loading