Release aggregate instances when sqlite finishes with them

[djmb]

The check at the top of the destroy function returns when the aggregate context still holds an instance, which is exactly when there is something to clean up. So instances were never unlinked, and every aggregate call left one behind for as long as the connection stayed open.

[djmb]

@flavorjones - here's a script to reproduce the leak. It creates a custom aggregator and then queries against it continually:

ruby sqlite3-aggregate-leak.rb                                    # released gem
SQLITE3_PATH=/path/to/sqlite3-ruby ruby sqlite3-aggregate-leak.rb # this branch

Before — sqlite3 2.9.5:

                         handlers      +/- slots         +/- MB
baseline                        1             +0           +0.0
after 200 queries          100001        +400036          +85.0
after 400 queries          200001        +400002         +124.0
after 600 queries          300001        +400006         +118.8
after 800 queries          400001        +400000         +126.1
after 1000 queries         500001        +400000         +124.7

Over 1000 queries: 500000 handlers leaked, 2000045 slots, +579 MB.
That is one handler per group per query (1000 x 500).

After — this branch:

                         handlers      +/- slots         +/- MB
baseline                        1             +0           +0.0
after 200 queries               1            +36          -19.4
after 400 queries               1             +2           +0.0
after 600 queries               1             +6           +0.0
after 800 queries               1             +0           +0.0
after 1000 queries              1             +0           +0.0

No leak: handlers stayed at 1.

sqlite3-aggregate-leak.rb

#!/usr/bin/env ruby
#
# Demonstrates that sqlite3-ruby never releases custom aggregate instances.
#
# sqlite creates one aggregate context per GROUP BY group, and the gem creates a
# handler instance per context. rb_sqlite3_aggregate_instance_destroy is supposed
# to unlink each one when sqlite calls xFinal, but its guard is inverted:
#
#   if (!inst_ptr || (inst = *inst_ptr)) {   /* returns when there IS an instance */
#       return;
#   }
#
# so the cleanup only runs when there is nothing to clean up. Every instance
# stays in the wrapper's -instances array for the life of the connection, along
# with whatever state the handler accumulated.
#
#   ruby sqlite3-aggregate-leak.rb                                  # released gem
#   SQLITE3_VERSION=2.9.4 ruby sqlite3-aggregate-leak.rb            # another release
#   SQLITE3_PATH=~/src/sqlite3-ruby ruby sqlite3-aggregate-leak.rb  # a local checkout
#
# Expected: 1 live handler (the template) no matter how many queries run.

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  if (path = ENV["SQLITE3_PATH"])
    gem "sqlite3", path: path
  else
    gem "sqlite3", ENV.fetch("SQLITE3_VERSION", "2.9.5")
  end
end

require "sqlite3"

ROWS = 50_000
GROUPS = 500
QUERIES = 1_000

# A realistic accumulating aggregate: it has to keep every value it sees, so a
# leaked instance retains an array rather than just an object header.
class Median
  def step(value)
    @values ||= []
    @values << value.to_f
  end

  def finalize
    sorted = @values.sort
    sorted[sorted.length / 2]
  end
end

def live_handlers
  GC.start(full_mark: true, immediate_sweep: true)
  ObjectSpace.each_object(Median).count
end

def live_slots
  GC.start(full_mark: true, immediate_sweep: true)
  GC.stat(:heap_live_slots)
end

def rss_mb
  File.read("/proc/self/status")[/VmRSS:\s+(\d+)/, 1].to_i / 1024.0
rescue StandardError
  nil
end

PREVIOUS = {}
START = {}

# Each row is the change since the row above it, so a steady figure means the
# leak is still going rather than having been a one-off cost.
def report(label)
  slots = live_slots
  rss = rss_mb
  slots_delta = PREVIOUS[:slots] ? slots - PREVIOUS[:slots] : 0
  rss_delta = rss && PREVIOUS[:rss] ? rss - PREVIOUS[:rss] : 0.0
  PREVIOUS[:slots] = slots
  PREVIOUS[:rss] = rss
  START[:slots] ||= slots
  START[:rss] ||= rss

  printf "%-22s %10d %14s %14s\n",
    label,
    live_handlers,
    format("%+d", slots_delta),
    rss ? format("%+.1f", rss_delta) : "n/a"
end

db = SQLite3::Database.new(":memory:")
db.execute "create table t (value real, grp integer)"
db.transaction do
  ROWS.times { |i| db.execute "insert into t values (?, ?)", [i * 1.5, i % GROUPS] }
end
db.define_aggregator "median", Median.new

puts "sqlite3 gem #{SQLite3::VERSION}, sqlite #{SQLite3::SQLITE_VERSION}, ruby #{RUBY_VERSION}"
puts "#{ROWS} rows over #{GROUPS} groups, #{QUERIES} queries"
puts

# Heap slots is the exact signal — four per leaked handler, linear from the first
# sample. Resident memory is the one that shows why it matters, though it lags
# for the first few dozen queries while bundler hands back what it allocated
# while resolving.
INTERVAL = QUERIES / 5

puts "slots and MB are the change since the row above, #{INTERVAL} queries earlier"
puts
printf "%-22s %10s %14s %14s\n", "", "handlers", "+/- slots", "+/- MB"
report "baseline"

QUERIES.times do |i|
  db.execute "select grp, median(value) from t group by grp"
  report "after #{i + 1} queries" if ((i + 1) % INTERVAL).zero?
end

puts
leaked = live_handlers - 1
if leaked.zero?
  puts "No leak: handlers stayed at 1."
else
  total_slots = live_slots - START[:slots]
  total_rss = rss_mb ? format(", %+.0f MB", rss_mb - START[:rss]) : ""
  puts "Over #{QUERIES} queries: #{leaked} handlers leaked, #{total_slots} slots#{total_rss}."
  puts "That is one handler per group per query (#{QUERIES} x #{GROUPS})."
  puts "Nothing is released until the Database object is garbage collected, so a"
  puts "pooled connection accumulates these for the life of the process."
end

[jeremy]

Approve. The diagnosis and the one-character fix are both right, and I reproduced the leak and its removal independently.

rb_sqlite3_aggregate_instance_destroy (ext/sqlite3/aggregator.c:100) reads

if (!inst_ptr || (inst = *inst_ptr)) {
    return;
}

inst is a VALUE, so (inst = *inst_ptr) is truthy exactly when an instance exists — the guard returns in the one case where there is something to release, and the body has been unreachable since the aggregate rewrite. Adding the ! restores the intent.

Verified locally

Ruby 4.0.6 (arm64-darwin23) and Ruby 3.4.10 / 4.0.5 (x86_64-linux), building each branch from source and loading that build rather than the installed precompiled gem.

Define an aggregator, run 50 aggregations, GC.start(full_mark: true, immediate_sweep: true), count live handler instances:

build	live handlers
main (07c92bc)	51
#722	1
#723 alone	51
#722 + #723	1

bundle exec rake test is clean over 10 consecutive runs on this branch, and the new test_aggregate_instances_are_released_after_each_query fails on main (Expected: 1, Actual: 6), so it does bite.

No CI has run on this branch

gh pr checks 722 reports "no checks reported on the 'fix/release-aggregate-instances' branch" and the PR sits at mergeStateStatus: BLOCKED, while #723 — filed the same day by the same author — has 135 green checks. @flavorjones, could you kick the workflows off here? This is the smaller and lower-risk of the two patches and it's the one with no signal.

Worth merging this before, or together with, #723

Not a correctness dependency — #723 stands on its own and is a clear net win either way. But #723's pin_aggregators walks each aggregator wrapper's -instances array on every GC mark and calls rb_gc_mark, the pinning mark, on every element. That is correct in itself: sqlite stores each live instance's VALUE inside sqlite3_aggregate_context() memory, which the collector never scans or relocates. But until this PR lands that array never drains, so the pinned set grows without bound.

The pinning is unambiguous. 200 queries × 200 groups on one connection, then sample 300 retained wrappers and compact:

build	retained wrappers	relocatable?
main	4000	yes — 300/300 sampled moved
#723 alone	4000	no — 0/300 moved, and 40,004 objects excluded from relocation entirely
#722 (+ #723)	0	n/a

Whether that costs anything depends on the workload, and I'd rather give you both results than just the flattering one. With a handler that retains an array per group — the Median in your benchmark above — retention dominates and pinning is invisible: 216 pages/68.1% occupancy on main vs 217/67.1% on #723. With a light handler whose wrappers end up scattered among collectable garbage, it shows clearly, at an essentially identical live set (~118,410 slots):

	pages after GC.compact	pages actually needed
main	125 → 94	91
#723 alone	121 → 111	88

So compaction recovers 31–40 pages on main and 10 on #723-alone, leaving ~18% more heap for the same live data. Merging this PR bounds the pinned set and the question goes away.

(For the record, and correcting something I expected to find and didn't: GC mark time is not amplified — ~18.6 ms/full-GC on main vs ~16.1 ms on #723 at 400 queries. The retention leak already pays that today.)

Nits

• This branch and #723 conflict textually in test/test_integration_aggregate.rb — both add a helper class and tests at the same two points. Trivial to resolve, keep both, but whoever merges second will hit it.

• No CHANGELOG.md entry. There's a ## next / unreleased section and comparable fixes (#710, #711) got one. Suggested, under ### Fixed:

  • Fix a leak where custom aggregate handler instances were never released, so a connection accumulated one instance per GROUP BY group per query for its lifetime. #722 @djmb