Skip to content

RAII things - #759

Open
cds-amal wants to merge 7 commits into
solana-foundation:mainfrom
cds-rs:spike/storage-backend
Open

RAII things#759
cds-amal wants to merge 7 commits into
solana-foundation:mainfrom
cds-rs:spike/storage-backend

Conversation

@cds-amal

@cds-amal cds-amal commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a SQLite connection leak that presents with exhausted file-descriptors over the course of multiple surfnet lifetimes, when the number of file-descriptors open crosses the system limit and file opens fail. This happens on stock linux machines under ulimit -Sn 1024.

Every SQLite store builds or borrows its pool from a process-global cache. A process accumulates connections for each surfnet because its associated connections are owned by the process-global cache. Further, every in-memory SQLite backed surfnet built 19 separate pools; one per store, each a cache miss (19 x 10).

The fix is to let StorageBackend give each surfnet an owner for its database connections, specifically one pool, built when the surfnet's storage initializes, shared by every store the surfnet opens. Stores hold pool clones so the pool-clones are bound to the store's lifetime, and when the StorageBackend goes away, it will release the one pool it owns; RAII ftw.

PostgreSQL holds a lease on the process-level pool for its database URL. Unlike SQLite, the pool stays shared across surfnets, since pooling exists to amortize the network connection and the server caps total sessions.

Measurements

Counter movement for ten surfnets built and dropped per phase, recorded by a temporary in-process census that this branch carried for the measurement and removes before merge (the history has it, the squash lands only the fix):

counter baseline this branch
on-disk: pools created 10, plus 180 cache reuses 10
on-disk: connections opened / closed 100 / 0 100 / 100
on-disk: connections live after the last drop 100 0
in-memory: pools created 190 10
in-memory: connections opened / closed 1,900 / 1,900 100 / 100
peak live connections 290 10

The suite-level effect: cargo test -p surfpool-core --lib --features ignore_tests_ci under ulimit -Sn 1024, same machine, same session,
both builds:

baseline this branch
result 536 passed, 105 failed 642 passed, 0 failed
wall clock 204.7s 36.1s
peak threads (3s samples) 4,954 1,079
peak fds (3s samples) 1,167 269
  • The wall-clock improvement is a consequence I didn't chase (number go down)
  • The residual 1,079 threads belong to r2d2 per-pool schedulers; I didn't go
    into that API, the ~5x drop was enough for this PR.

Verifying from outside the process

The on-disk result reproduces externally, with no code from this branch: the workload is 31 pre-existing tests that each build and drop an on-disk surfnet.

BIN=$(cargo test -p surfpool-core --lib --no-run --message-format=json 2>/dev/null \
  | jq -r 'select(.reason=="compiler-artifact" and .executable != null) | .executable')
"$BIN" with_on_disk_sqlite_db --skip integration --test-threads=1 &
PID=$!
while kill -0 $PID 2>/dev/null; do
  # Linux; on macOS use: lsof -p $PID | grep -c sqlite
  find /proc/$PID/fd -lname '*sqlite*' 2>/dev/null | wc -l
  sleep 0.2
done

On main the count climbs a staircase to 434 and holds it until process exit; on this branch it peaks at 23 (one live surfnet's worth) and returns to the floor between tests. In-memory databases hold no descriptors, so that half of the improvement is visible only in the counter table above.

@cds-amal
cds-amal force-pushed the spike/storage-backend branch from cd8a29c to 52a396e Compare August 15, 2026 18:10
cds-amal and others added 7 commits August 15, 2026 18:28
Process-wide atomic counters for pool and connection events, a
snapshot/since API, and an ignored census_workload test that builds and
drops ten surfnets per phase and prints the movement. Nothing is
connected yet; the counters read zero until a backend routes its pool
construction and connections through them. Closes are counted by Drop on
a connection wrapper around the r2d2 manager, because
CustomizeConnection::on_release fires only for broken or reaped
connections, not at pool drop, and undercounts.

Co-authored-by: Claude <noreply@anthropic.com>
SQLite pools now build through CountingSqliteManager, and the build sites
in get_or_create_shared_pool report creation and cache reuse.

census_workload, ten surfnets built and dropped per phase:

    on-disk:   pools 10 (+180 reuses), conns opened 100, closed 0,
               100 still live after the last drop
    in-memory: pools 190, conns opened 1,900, closed 1,900,
               peak 190 live during one surfnet

Connections owned by the process-global SHARED_POOLS survive every
surfnet drop. The in-memory line prices the 19 isolated pools each
surfnet builds.

Co-authored-by: Claude <noreply@anthropic.com>
…ions

Each surfnet now opens one StorageBackend from its database URL and mints
all of its kv stores from it. The SQLite pool moves off the process-global
SHARED_POOLS map onto the backend, so connections live exactly as long as
the surfnet that opened them; the checkpoint-dedup set and the per-store
shutdown fan-out fall away with it. PostgreSQL keeps its process-level
pool cache behind the backend, where pooling actually amortizes a network
connection. In-memory SQLite now shares one database per surfnet instead
of building 19 isolated pools, matching the on-disk layout of distinct
tables in one database.

The counting manager remains the pool's connection type, so connection
opens and closes stay counted; the census build-site hook is reconnected
in the next commit.

Co-authored-by: Claude <noreply@anthropic.com>
SqliteBackend::open reports pool creation, and a new assertion test,
dropping_backend_closes_connections, fails if any connection opened by a
backend survives its drop.

census_workload, same workload as the baseline measurement two commits
back:

    on-disk:   pools 10, conns opened 100, closed 100, live 0
    in-memory: pools 10, conns opened 100, closed 100, live 0

Ten dropped surfnets leave 100 connections live on the baseline and none
here; in-memory construction opens 190 connections per surfnet there and
10 here.

Co-authored-by: Claude <noreply@anthropic.com>
Counting stays always on: the counters are a handful of relaxed atomics
bumped at pool and connection lifecycle events, which are rare next to
queries, so there is nothing worth stripping from a release build. What
was missing was a way to see the movement outside a test harness.

Each counter now emits a debug line under this module's log target, so
RUST_LOG=surfpool_core::storage::census=debug turns on reporting in any
build, including shipped binaries. That matters for field diagnosis:
when someone hits descriptor exhaustion, the ask is an env var and a
re-run, not a from-source build. It also keeps a single pool type in
every configuration, so tests exercise exactly what ships.

The census tests initialize env_logger (is_test, same pattern as the
integration suite) so harness runs show the per-event narrative next to
the snapshot deltas.

Note the event ordering in the logs: "pool created" appears after its
connections open because r2d2 fills the pool eagerly inside build(),
and we count the pool only once the build succeeds.
The census did its job: the baseline and fix measurement commits carry
its numbers, and an external fd sampler reproduces the on-disk result
from outside the process (peak 434 SQLite descriptors on the shared-pool
baseline against 23 with backend-owned pools, over the same workload of
31 on-disk tests). The evidence for the refactor no longer needs
counters in the tree, and keeping them would mean a counting wrapper
inside the shipped pool type forever, a larger maintenance surface than
a merged fix should carry.

SqliteBackend builds on a plain ConnectionManager again, so the pooled
connection loses one deref hop and the pool type is identical in every
configuration. The pre-removal tip is tagged census-archive; the
counters, the Drop-based close accounting with its r2d2 on_release
subtlety, and the workload harness live there if this leak class ever
needs exact instrumentation again.
@cds-amal
cds-amal force-pushed the spike/storage-backend branch from 52a396e to de71c83 Compare August 15, 2026 22:30
@cds-amal
cds-amal marked this pull request as ready for review August 16, 2026 01:46
@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR replaces per-store database construction with a surfnet-owned StorageBackend, allowing all SQLite stores in a surfnet to share one lifecycle-bound pool while PostgreSQL continues leasing its process-shared pool.

  • Adds backend abstractions that create typed stores from a shared SQLite or PostgreSQL pool.
  • Migrates SurfnetSvm and SurfnetLiteSvm storage initialization and shutdown to the backend owner.
  • Updates SQLite pooling and in-memory isolation tests for the per-backend ownership model.

Confidence Score: 5/5

The PR appears safe to merge; no concrete changed-code-triggered failure remains.

The storage lifecycle refactor consistently routes store creation through a surfnet-owned backend, preserves PostgreSQL sharing, and keeps bundle sandbox pool clones aligned with the lifetimes of their overlay stores.

Important Files Changed

Filename Overview
crates/core/src/storage/backend.rs Introduces the surfnet-scoped backend owner and centralizes store construction and SQLite shutdown.
crates/core/src/storage/sqlite.rs Moves SQLite pool ownership to SqliteBackend, shares one pool among a surfnet's stores, and preserves isolated named in-memory databases between backends.
crates/core/src/storage/postgres.rs Adds a backend lease over the existing process-shared PostgreSQL pool and constructs stores from that lease.
crates/core/src/surfnet/svm.rs Initializes all SVM stores from one backend, carries the backend into profiling clones, and consolidates shutdown through it.
crates/core/src/surfnet/surfnet_lite_svm.rs Opens persistent account storage through the shared backend and removes per-store shutdown responsibility.
crates/core/src/storage/mod.rs Exports StorageBackend and removes the superseded URL-based per-store constructors.

Reviews (1): Last reviewed commit: "Remove the census now that its measureme..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant