Stop sqlite calling into moved Ruby objects

[djmb]

When you register a scalar function, aggregator, collation, trace handler or authorizer, we hand sqlite a pointer to a Ruby object. The garbage collector can move that object without updating sqlite's copy of the address.

The database's C struct now points at the collections holding those callbacks, so the mark function can pin them.

Trace and authorizer passed the database and read the callback out of an instance variable. They now pass the struct instead, which lives in malloc'd memory the collector never relocates. The busy handler already worked this way.

Writes to the new fields go through RB_OBJ_WRITE so the collector keeps them alive.

There is a test for each of the five that forces every movable object to move. None of them hold the object under test in a local variable, which would prevent that.

[djmb]

@flavorjones - I found this issue because I was getting random errors and crashes from a Rails app that used custom functions running parallel tests against SQLite.

I got Claude + Codex to then track down the other instances of unpinned objects that the GC could move.

[jeremy]

Approve the approach. The bug is real, the diagnosis is right, and I verified each of the five sites independently before and after. Everything below is either a merge-order note or a cleanup — nothing blocking.

Verified locally

Ruby 4.0.6 (arm64-darwin23), plus Ruby 4.0.5 (x86_64-linux) and 3.4.10 under valgrind. Each branch built from source and loaded from that build. Every repro registers its callback without holding it in a local — a local is conservatively pinned by the machine-stack scan and masks the bug — then calls GC.verify_compaction_references(expand_heap: true, toward: :empty).

repro	main	#722	#723	#722+#723
create_function	SEGV	SEGV	pass	pass
define_aggregator	SEGV	SEGV	pass	pass
collation	NoMethodError	NoMethodError	pass	pass
trace	NoMethodError	NoMethodError	pass	pass
authorizer	NoMethodError	NoMethodError	pass	pass

The failure signatures confirm the mechanism you describe. trace and authorizer give undefined method 'call' for nil — they passed (void *)self and then rb_iv_get the callback, so when the Database moves the ivar read lands on whatever now occupies the address. collation gives undefined method 'compare' for an instance of #<#<Class:0x...>:0x...>, i.e. the stale VALUE resolved to an unrelated live object. Moving trace and authorizer to (void *)ctx is the right fix and matches busy_handler.

On the combined branch: bundle exec rake test is 344 runs, 683 assertions, 0 failures, 0 skips, clean over 10 consecutive runs on macOS and on Linux — so force_gc_compaction really is returning true on 4.0.x and the new tests run rather than skip. rake test:valgrind (ruby_memcheck 3.0.1, valgrind 3.24, ruby 3.4.10) is clean: 344 runs, 0 failures, 0 errors, and the only 5 skips are the pre-existing i_am_running_in_valgrind fork/discard ones in test_discarding.rb. A pinning change seemed worth putting under memcheck.

I also ran a negative control: 200 rounds of ordinary GC.compact with allocation churn, against a connection carrying scalar functions, an aggregate and a collation. It passes on main too — ordinary compaction doesn't reliably relocate these callbacks, and forced compaction is what makes the bug deterministic. Nothing here changes that; it just means this PR shouldn't be read as explaining any particular intermittent field report.

1. Worth merging #722 first, or together with this

#722 is a one-character fix for an inverted guard in rb_sqlite3_aggregate_instance_destroy (ext/sqlite3/aggregator.c:100) that means aggregate instances are never released. It interacts with this PR: pin_aggregators walks each wrapper's -instances array on every GC mark and pins every element — correct, since sqlite keeps each live instance's VALUE in sqlite3_aggregate_context() memory that the collector never scans — but until #722 lands that array never drains, so the pinned set is unbounded.

This isn't a correctness dependency; this PR is a clear improvement on its own. But it does convert an unbounded movable retention leak into an unbounded pinned one. The pinning itself is unambiguous — 200 queries × 200 groups, sample 300 retained wrappers, compact:

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

What it costs is workload-dependent, and both results seem worth having. With a handler that retains an array per group (the Median in #722's benchmark) retention dominates and pinning is invisible: 216 pages / 68.1% occupancy on main vs 217 / 67.1% on this branch. 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

Compaction recovers 31–40 pages on main and 10 here, leaving ~18% more heap for the same live data. Since the whole point of this PR is to make the extension safe for applications that compact, it seems a shame to ship it in a state where it also makes part of their heap uncompactable. #722 bounds the pinned set and the question disappears.

One claim I expected to be able to make and could not: GC mark time is not amplified — ~18.6 ms/full-GC on main vs ~16.1 ms here at 400 queries. The retained object graph already costs that today.

Neither PR mentions the other; worth a line in both descriptions.

2. Pinning vs. relocating — worth stating explicitly

This PR pins and adds no dcompact. That matches the busy_handler precedent from #466 (rb_gc_mark, not rb_gc_mark_movable), and since the collections are also referenced from the struct, pinning the containers is what lets it skip dcompact entirely. Coherent, but a reader currently has to infer it — worth a sentence in the description. With #722 merged the pinned set is bounded by registered callbacks plus in-flight aggregations, which is the right size.

3. Smaller things

• Redundant ivars. @tracefunc (database.c:333) and @authorizer (database.c:730) are now written both to the ivar and to the struct. Neither has a public reader — attr_reader :collations is the only one on SQLite3::Database. #466 left the same duplication behind and f759e82 removed @busy_handler afterwards; probably nicer to do it here than to leave a second cleanup commit. (@progress_handler, lib/sqlite3/database.rb:176, looks dead already — separate issue, just noting it.)

• pin_hash_value unused parameters. The repo has an UNUSED() macro (ext/sqlite3/sqlite3_ruby.h:6, used at database.c:601). Cosmetic; nothing warns today.

• exec_batch. ext/sqlite3/database.c:895-901 still hands sqlite3_exec a raw VALUE (callback_ary). I tried to break it — 300,000 rows through execute_batch2 under GC.auto_compact = true with interleaved GC.compact — and couldn't, which is what you'd expect: it's a C local live across the whole synchronous call, so the conservative stack scan both marks and pins it. But after this PR it's the last raw-VALUE handoff in the extension, and a one-line comment saying why it's exempt would save the next audit from re-deriving that.

• Test gap. There's a compaction test for all five newly-fixed sites but not for busy_handler, which #466 fixed without one. One more test closes the set and guards the old fix against this one.

• test_replacing_a_collation_releases_the_previous_comparator passes on main too. The other seven new tests all fail there (3 NoMethodError, 2 SEGV, 1 count). That one is a regression guard for the design choice rather than a test of the fix — fine, and it usefully rules out an append-only array for @collations, but worth a comment saying so, since a test that passes on both sides otherwise reads as dead weight.

• rb_hash_foreach in a dmark. Public API, but unusual inside a mark function — on a non-frozen hash it bumps the hash's iteration level and wraps the walk in rb_ensure. I went looking for a failure (200 collations interleaved with forced marks, then marks and a compaction from inside a Ruby-level db.collations.each) and found none, and valgrind is clean, so this is a curiosity rather than a request — ignore unless it bothers you too.

• CHANGELOG. No entry; there's a ## next / unreleased section and #710/#711 got one. Suggested, under ### Fixed:

  • Fix GC compaction issues with custom functions, aggregates, collations, #trace and #authorizer=. These callbacks were registered with sqlite by passing a raw Ruby object pointer as user data; keeping the object reachable prevented collection but not relocation, after which sqlite held a stale address and the next call could raise NoMethodError, return a wrong result, or segfault. Affects applications that call GC.compact or run with GC.auto_compact = true. The equivalent issue in #busy_handler was fixed in #466. #723 @djmb

• Minor: this branch conflicts textually with #722 in test/test_integration_aggregate.rb. Trivial — keep both.

[jeremy]

One more data point since I had the harness set up: the suite is also clean under UBSan. Extension built with -fsanitize=undefined -fno-sanitize-recover=all passed through --with-cflags, so only this extension is instrumented and the vendored sqlite keeps stock flags (--with-sqlite-cflags is a separate seam) — 344 runs, 0 failures, 0 runtime errors.

ASan is not usable here without more work, for what it is worth: with an uninstrumented ruby it intercepts the GC page munmap and dies with AddressSanitizer failed to deallocate ... (error code: 22) before the suite finishes. That would need a ruby built with ASan, which seemed out of proportion given memcheck and UBSan are both clean.

[jeremy]

Follow-ups on top of the approval — refinements, not objections. I ran the branch with all of these applied: 344 runs, 679 assertions, 0 failures, 0 errors, 0 skips, and rubocop clean over 42 files. The zero skips is the part worth stating: it means force_gc_compaction actually compacted rather than bailing out, so the five new tests really did exercise the fix.

Four are per-line suggestions below. The other four touch lines outside this PR's diff hunks, so GitHub won't anchor a suggestion to them — they're inline here.

A. ext/sqlite3/database.c — say why exec_batch is allowed to keep its raw VALUE

After this change, exec_batch holds the only remaining (void *) cast of a raw Ruby VALUE in the extension, which makes it look like a spot the sweep missed. It isn't, but the reasoning is non-obvious enough to be worth writing down — both so the next person auditing for this bug class doesn't have to re-derive it, and so nobody "fixes" it into the struct and pays for a pin that isn't needed.

Immediately above if (results_as_hash == Qtrue) { (line 952):

    /* The one remaining place we hand sqlite a raw VALUE, and the only one that
     * is safe: callback_ary is a live C local for the whole of the synchronous
     * sqlite3_exec, so the conservative machine stack scan both marks and pins
     * it. sqlite drops the pointer before this function returns. Registered
     * callbacks - functions, aggregates, collations, trace, authorizer - outlive
     * their registering call and so have to be pinned through the struct
     * instead. */

B. lib/sqlite3/database.rb:174-176 — drop the dead ivar initialisers

Pairs with the two rb_iv_set removals suggested below. Once those go, @tracefunc and @authorizer have no readers left anywhere: tracefunc() reads ctx->trace_handler and rb_sqlite3_auth reads db_ctx->authorizer.

@progress_handler is a different case, and worth being precise about: it is already dead on main. sqlite3_progress_handler at ext/sqlite3/database.c:374 passes (void *)ctx and never touches the ivar, and nothing under lib/ or ext/ reads it. So this PR doesn't orphan it — it's pre-existing dead code that happens to sit next to the two this PR does orphan. Removing two of three siblings and leaving the third reads as "that one must still matter", so closing it out here keeps the block honest. Entirely reasonable to split it into a separate commit if you'd rather keep this PR to one subject.

-      @tracefunc = nil
-      @authorizer = nil
-      @progress_handler = nil
       @collations = {}
       @functions = []

C. test/test_integration_pending.rb — bring busy_handler under the same compaction test

The busy handler has been reached through the struct since #466, so this passes on main as-is. The value is that all six struct-held callbacks then share one test shape, and a future regression in the #466 fix gets caught by the same net as the five this PR moves over.

One ordering detail worth keeping if you take this: skip runs before the helper thread is created, so on a runtime that can't compact there's no thread holding an exclusive lock on test.db left to unwind — which is also what keeps Minitest/SkipEnsure quiet. Verified both ways: rubocop --only Minitest/SkipEnsure is clean, and the file runs 5 tests / 0 skips.

Insert after test_busy_handler_impatient:

  # The busy handler has been reached through the struct since #466, so this
  # passes on main too. It is here to keep that fix covered by the same
  # compaction test as the five this change moves over.
  def test_busy_handler_does_not_use_moved_block_after_gc_compaction
    # Doubles as the support probe, so we decide whether to skip before there is
    # a helper thread holding an exclusive lock on test.db to clean up.
    skip("GC compaction is unsupported on this runtime") unless force_gc_compaction

    synchronizer = ThreadSynchronizer.new
    handler_call_count = 0

    t = Thread.new(synchronizer) do |sync|
      db2 = SQLite3::Database.open("test.db")
      db2.transaction(:exclusive) do
        sync.send_to_main :ready_0
        sync.wait_for_main :end_1
      end
    ensure
      db2&.close
      sync.close_thread
    end
    synchronizer.wait_for_thread :ready_0

    @db.busy_handler do
      handler_call_count += 1
      false
    end

    force_gc_compaction

    assert_raise(SQLite3::BusyException) do
      @db.execute "insert into foo (b) values ( 'from 2' )"
    end
    assert_equal 1, handler_call_count

    synchronizer.send_to_thread :end_1
    synchronizer.close_main
    t.join
  end

D. CHANGELOG.md — a ### Fixed entry

Deliberately not a suggestion: it's a whole new section, and a block spanning the new header plus the existing ### Improved would be awkward to apply. Goes directly under ## next / unreleased, above ### Improved:

### Fixed

- Fix GC compaction issues with custom functions, aggregates, collations, `#trace` and `#authorizer=`. These callbacks were registered with sqlite by passing a raw Ruby object pointer as user data; keeping the object reachable prevented collection but not relocation, after which sqlite held a stale address and the next call could raise `NoMethodError`, return a wrong result, or segfault. Affects applications that call `GC.compact` or run with `GC.auto_compact = true`. The equivalent issue in `#busy_handler` was fixed in #466. #723 @djmb

The symptom list is the load-bearing part — NoMethodError, wrong result, or segfault is what someone who has hit this in production will be searching for.

[jeremy]

key and arg are unused, so this trips -Wunused-parameter. The extension already has a convention for exactly this — the UNUSED macro from sqlite3_ruby.h, used a few hundred lines down at complete_p (line 656).

Suggested change

	pin_hash_value(VALUE key, VALUE value, VALUE arg)
	pin_hash_value(VALUE UNUSED(key), VALUE value, VALUE UNUSED(arg))

[jeremy]

The rb_iv_set is dead now: tracefunc() reads ctx->trace_handler, and nothing anywhere reads @tracefunc. Leaving it implies a second source of truth for the handler, which is the exact confusion the struct move is clearing up — and it's the kind of line that survives a decade because it looks load-bearing.

Suggested change

	rb_iv_set(self, "@tracefunc", block);
	RB_OBJ_WRITE(self, &ctx->trace_handler, block);
	RB_OBJ_WRITE(self, &ctx->trace_handler, block);

The matching @tracefunc = nil in lib/sqlite3/database.rb goes with it — note B in the review body.

[jeremy]

Same as @tracefunc: rb_sqlite3_auth reads db_ctx->authorizer now, so the ivar write has no reader.

Suggested change

	rb_iv_set(self, "@authorizer", authorizer);
	RB_OBJ_WRITE(self, &ctx->authorizer, authorizer);
	RB_OBJ_WRITE(self, &ctx->authorizer, authorizer);

[jeremy]

This test passes on main too, so someone bisecting a future failure will reasonably wonder what it's guarding. It guards the design rather than the fix: now that every comparator gets pinned, the fact that @collations is a hash keyed by name — and so drops the previous comparator on replace — becomes load-bearing. Make it an append-only array and you have an unbounded pinned set. Without a note, this is the kind of test that gets deleted as redundant by the next person who runs it against main.

Suggested change

	def test_replacing_a_collation_releases_the_previous_comparator
	# Passes on main as well. It guards the design rather than the fix: pinning
	# every comparator makes the lifetime of @collations matter, so this rules
	# out ever making it an append-only array.
	def test_replacing_a_collation_releases_the_previous_comparator