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

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

Are we sure this approach actually works?

I'm worried what happens if sqlite does a lot of caching, and evicts the cache as needed. Worker A does a query, populating the cache with Worker A's database pages. Worker B does a query that requires a lot of memory. This causes Worker A's pages to be evicted. But because Worker B is the one running at that time, the evictions are counted as memory deallocation on behalf of Worker B, even though the pages were originally allocated by Worker A.

From the SQLite core team each database within a database connection operates independently of all the others. As long as we do not do the following things (which we do not):

• We do not using shared-cache mode (forcibly disabled using -DSQLITE_OMIT_SHARED_CACHE in this PR).
• We do not allocate separate cache memory via sqlite3_config(SQLITE_CONFIG_PCACHE).
• We do not used sqlite3_config(SQLITE_CONFIG_PCACHE2) to implement our own customized page cache algorithm.
• We do not build SQLite using SQLITE_ENABLE_MEMORY_MANAGEMENT.
  We can assume that one connection will never free pages from another connection's page cache.

Codecov Report

❌ Patch coverage is 77.35294% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.83%. Comparing base (6fc69a6) to head (a09e565).
⚠️ Report is 9 commits behind head on main.

@@            Coverage Diff             @@
##             main    #6380      +/-   ##
==========================================
+ Coverage   70.81%   70.83%   +0.01%     
==========================================
  Files         435      437       +2     
  Lines      122668   123224     +556     
  Branches    19268    19393     +125     
==========================================
+ Hits        86872    87283     +411     
- Misses      24383    24439      +56     
- Partials    11413    11502      +89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

FWIW, it looks like the windows test failures happen because the sqlite build process ends up generating a file named "VERSION" with the raw sqlite version in it, and then other dependencies like boringssl fail when they try to do #include <version>, because sqlite's "VERSION" file is somehow in the include path and Windows is using a case-insensitive file system.

Since the windows tests build seems to be passing on main, I assume the failure is due to something in this PR. But since the only thing this PR changes about the sqlite3 build process is the "SQLITE_OMIT_SHARED_CACHE" flag, I assume the issue is only arising because the PR needs to rebuild sqlite because of the change. I'm not sure whether we saw the same problems on earlier sqlite3 build changes or what we might have done to resolve them.

Merging this PR will not alter performance

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks¹

_{Comparing jhoward/STOR-5096 (a09e565) with main (9dbd5fa)}