Attribute SQLite memory consumed to isolate memory consumed

[joshthoward]

No description provided.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds per-actor SQLite memory metering by installing a custom sqlite3_mem_methods allocator that routes accounting through jsg::ExternalMemoryAdjustment via a thread-local RAII scope.

Issues (highest severity first)

1. [High] sqliteMemFree can assert-fail when freeing memory allocated outside a scope. SQLite may allocate memory outside any SqliteMemoryScope (e.g., during sqlite3_open_v2() in the ActorSqlite constructor, before any JS turn installs a scope). When that memory is later freed during a JS turn (inside a scope), sqliteMemFree will decrement the adjustment by the full block size — even though the adjustment was never incremented for that allocation. This calls ExternalMemoryAdjustment::adjust(-N) which invokes maybeDeferAdjustment() containing KJ_ASSERT(amount >= -static_cast<ssize_t>(this->amount)). Since this->amount could be 0 (or less than N), this will assert-fail. The symmetric case (allocated in scope, freed outside scope) causes memory to leak from accounting but won't crash.

2. [High] No tests. This is a new subsystem that replaces the process-wide SQLite allocator and intercepts every malloc/free/realloc call made by SQLite. A unit test exercising the accounting (allocations tracked, frees decremented, realloc delta correct, no-scope passthrough) would catch issues like #1 above and prevent regressions.

3. [Medium] sqliteMemRealloc under-accounts when oldActual is 0 but scope is active. On line 90, the condition scope != nullptr && oldActual > 0 means if ptr was nullptr (realloc-as-malloc) or usableSize returned 0 for the old pointer, the new allocation is never accounted for. While SQLite internally redirects realloc(NULL, n) to malloc(n), the defensive guard still creates an inconsistency: if usableSize returns 0 for a valid old pointer (the #else fallback), the new allocation's size is also lost. The fix is to account for the new allocation even when oldActual is 0.

4. [Medium] adjustment is a public data member on SqliteMemoryScope. The KJ style convention is to minimize public surface area. This reference is only accessed by the three friend allocator functions. Making it private (the friends already have access) would prevent misuse.

5. [Low] Stale comment referencing nonexistent ensureMonkeyPatchedSqlite. The comment on line 138 of sqlite-metering.c++ says "We rely on callers (Vfs constructors, ensureMonkeyPatchedSqlite)" but ensureMonkeyPatchedSqlite does not exist in the codebase.

6. [Low] Comment references nonexistent SQLITE_MEMORY_METERING autogate. The header comment on line 19 of sqlite-metering.h mentions a SQLITE_MEMORY_METERING autogate, but no such autogate exists in autogate.h. If this feature is meant to be gated behind an autogate for staged rollout (which seems wise given it replaces the global SQLite allocator), the autogate should be added in this PR. If it's planned for a follow-up, the comment should say so.

7. [Low] Unnecessary #include <workerd/jsg/setup.h> in actor-sqlite.c++. The ExternalMemoryTarget class is fully defined in jsg/jsg.h (which is already transitively included via actor-cache.h). The setup.h header pulls in significantly more than needed.

[ask-bonk]

Assertion risk: If memory was allocated outside a SqliteMemoryScope (e.g., during sqlite3_open_v2() in the ActorSqlite constructor), but freed here inside a scope during a JS turn, this will call adjustment.adjust(-actual) when the adjustment's internal amount may be 0 or smaller than actual. That triggers KJ_ASSERT(amount >= -static_cast<ssize_t>(this->amount)) in ExternalMemoryAdjustment::maybeDeferAdjustment().

Consider tracking only the delta (allocs minus frees) within the scope's lifetime, or guarding against going negative:

Suggested change

	std::free(ptr);
	SqliteMemoryScope* scope = SqliteMemoryScope::threadLocalScope;
	if (scope != nullptr) {
	size_t actual = usableSize(ptr);
	if (actual > 0 && scope->adjustment.getAmount() >= actual) {
	scope->adjustment.adjust(-static_cast<ssize_t>(actual));
	}
	}
	std::free(ptr);

Alternatively, consider using a separate counter that allows negative drift (e.g., a plain std::atomic<ssize_t>) instead of piping raw alloc/free sizes through ExternalMemoryAdjustment which asserts non-negative.

[ask-bonk]

Under-accounting: When oldActual is 0 (either because the #else fallback is hit for the old pointer, or for any unexpected reason), the new allocation's full size is silently dropped from accounting. This makes the guard oldActual > 0 too strict — it conflates "I don't know the old size" with "don't track the new size either."

Consider accounting for the new allocation regardless:

Suggested change

	}
	if (scope != nullptr) {
	size_t newActual = usableSize(newPtr);
	ssize_t delta = static_cast<ssize_t>(newActual) - static_cast<ssize_t>(oldActual);
	if (delta != 0) {
	scope->adjustment.adjust(delta);
	}
	}
	return newPtr;

This handles all cases: if oldActual is 0 (unknown or untracked), the full new size is accounted; if both are known, the delta is correctly tracked.

[ask-bonk]

This data member is only accessed by the three friend functions declared below. Making it private would be more consistent with KJ conventions and prevent unintended external access:

Suggested change

	jsg::ExternalMemoryAdjustment& adjustment;
	private:
	jsg::ExternalMemoryAdjustment& adjustment;

(The existing friend declarations already grant the allocator functions access.)

[ask-bonk]

Stale reference: ensureMonkeyPatchedSqlite does not exist in the codebase. Did you mean a different function, or is this a reference to an internal-only caller?

Suggested change

	// We rely on callers (Vfs constructors, ensureMonkeyPatchedSqlite) to call this before any of
	// We rely on callers (Vfs constructors) to call this before any of

[ask-bonk]

This comment references a SQLITE_MEMORY_METERING autogate that doesn't exist in autogate.h. Given that this PR replaces the global SQLite allocator for the entire process, gating behind an autogate seems prudent for staged rollout. If the autogate is planned for a follow-up PR, it would be helpful to note that here.

[ask-bonk]

This include may be unnecessary — ExternalMemoryTarget and ExternalMemoryAdjustment are fully defined in jsg/jsg.h, which is already included transitively via actor-cache.h → jsg/jsg.h. setup.h is a heavy header that pulls in the full isolate/V8 setup machinery.

[ask-bonk]

Review posted successfully on PR #6380. Here's a summary of what I found:

7 issues identified, with the two highest severity being:

1. [High] Assertion crash risk in sqliteMemFree — Memory allocated by SQLite outside a SqliteMemoryScope (e.g., during sqlite3_open_v2()) that is later freed inside a scope will call adjust(-N) when the adjustment's tracked amount may be 0, triggering a KJ_ASSERT failure in ExternalMemoryAdjustment::maybeDeferAdjustment().

2. [High] No tests — This replaces the global SQLite allocator for the entire process. A unit test covering the basic accounting paths would catch the assertion issue and prevent regressions.

3. [Medium] Realloc under-accounting when oldActual is 0 but scope is active.

4. [Medium] Public data member on SqliteMemoryScope that should be private (friends already have access).

5-7. [Low] Stale comment referencing nonexistent ensureMonkeyPatchedSqlite, comment referencing nonexistent SQLITE_MEMORY_METERING autogate, and unnecessary #include <workerd/jsg/setup.h>.

All issues with concrete fixes were posted as inline suggestion comments on the PR.

github run

[github-actions]

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run just generate-types to update the snapshot. Alternatively, you can download the full generated types:

Full Type Diff