Skip to content

feat(node-config): add SQL connection pool tuning env vars#139

Merged
rswanson merged 3 commits intomainfrom
feat/cold-sql-pool-config
Apr 16, 2026
Merged

feat(node-config): add SQL connection pool tuning env vars#139
rswanson merged 3 commits intomainfrom
feat/cold-sql-pool-config

Conversation

@rswanson
Copy link
Copy Markdown
Member

Summary

  • Add 5 environment variables to StorageConfig for tuning the SQL cold storage connection pool: SIGNET_COLD_SQL_MAX_CONNECTIONS, SIGNET_COLD_SQL_MIN_CONNECTIONS, SIGNET_COLD_SQL_ACQUIRE_TIMEOUT_SECS, SIGNET_COLD_SQL_IDLE_TIMEOUT_SECS, SIGNET_COLD_SQL_MAX_LIFETIME_SECS
  • Defaults tuned for RPC workloads (100 max connections, 5 min, 5s acquire timeout, 10m idle, 30m lifetime) — previously sqlx defaults of 10 max connections and 30s acquire timeout caused pool starvation under moderate load
  • Bumps signet-storage dependencies from 0.6.5 to 0.7 to use the SqlConnector builder methods

Context

The sidecar RPC's cold storage task spawns up to 64 concurrent readers, all sharing the sqlx pool. With the previous default of 10 connections, 54 readers would block waiting for a free connection (up to the 30s acquire timeout). Combined with the write-drain behavior (every new block drains all in-flight reads before writing), this created cascading stalls under even moderate load.

Test plan

  • cargo +nightly fmt -- --check
  • cargo clippy -p signet-node-config --all-targets (default features)
  • cargo clippy -p signet-node-config --all-targets --features postgres
  • cargo t -p signet-node-config
  • RUSTDOCFLAGS="-D warnings" cargo doc -p signet-node-config --no-deps
  • Deploy to dev cluster with SIGNET_COLD_SQL_MAX_CONNECTIONS=100 and verify pool starvation is resolved

🤖 Generated with Claude Code

The SQL cold storage pool previously used sqlx defaults (10 max
connections, 30s acquire timeout), causing pool starvation under
moderate RPC load when 64 concurrent cold-storage readers compete
for connections.

Add five new environment variables to StorageConfig for tuning the
sqlx pool: max/min connections, acquire timeout, idle timeout, and
max lifetime. Defaults are tuned for RPC workloads (100 max, 5 min,
5s acquire, 10m idle, 30m lifetime).

Also bumps signet-storage dependencies from 0.6.5 to 0.7 to pick up
the SqlConnector builder methods.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rswanson rswanson requested a review from a team as a code owner April 16, 2026 12:31
@rswanson rswanson self-assigned this Apr 16, 2026
@rswanson
Copy link
Copy Markdown
Member Author

Code review

Found 1 issue:

  1. pub(super) used on 5 constants in pool_defaults module (CLAUDE.md says "Private by default, pub(crate) for internal, pub for API only; never pub(super)")

/// Maximum number of connections in the SQL cold storage pool.
pub(super) const MAX_CONNECTIONS: u32 = 100;
/// Minimum number of connections in the SQL cold storage pool.
pub(super) const MIN_CONNECTIONS: u32 = 5;
/// Timeout (in seconds) for acquiring a connection from the pool.
pub(super) const ACQUIRE_TIMEOUT_SECS: u64 = 5;
/// Idle timeout (in seconds) before closing unused connections.
pub(super) const IDLE_TIMEOUT_SECS: u64 = 600;
/// Maximum lifetime (in seconds) of individual connections.
pub(super) const MAX_LIFETIME_SECS: u64 = 1800;
}

The pool_defaults module constants use pub(super) visibility, which is explicitly prohibited by the project CLAUDE.md. Since these constants are only consumed by the parent module's build_sql_connector() method, they should use pub(crate) instead.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

CLAUDE.md prohibits pub(super). Use pub(crate) for the pool_defaults
module constants.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

work belongs in bin base. add an optional dep on subset storage and a fromenv impl for the connector. @Fraser999 has context and there may already be a ticket

this pr then becomes adding a connector to the config

additional pr needed to add serde to the connector in storage repo

Copy link
Copy Markdown
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a couple of non-blocking points.

Comment thread crates/node-config/src/storage.rs Outdated
Comment thread crates/node-config/src/storage.rs Outdated
Copy link
Copy Markdown
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version version bump of deps contains unrelated behavior changes?

@Fraser999
Copy link
Copy Markdown
Contributor

@prestwich

version bump of deps contains unrelated behavior changes?

The new version of bin-base depends upon storage 0.7.1

@rswanson rswanson enabled auto-merge (squash) April 16, 2026 16:20
@rswanson rswanson merged commit 4d22eef into main Apr 16, 2026
13 of 14 checks passed
Copy link
Copy Markdown
Contributor

Fraser999 commented Apr 16, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

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.

3 participants