feat(db): Postgres storage backend for evmhot (fixes chronic write-queue saturation) - #12
Open
S0c5 wants to merge 8 commits into
Open
feat(db): Postgres storage backend for evmhot (fixes chronic write-queue saturation)#12S0c5 wants to merge 8 commits into
S0c5 wants to merge 8 commits into
Conversation
…acade
Splits the monolithic src/db.rs into a db/ module: db/sqlite.rs keeps the
existing rusqlite-backed Db (writer actor, priority lanes, WAL checkpoint
logic) completely unchanged, and the new db/mod.rs introduces a thin Db
facade with `enum DbInner { Sqlite, Postgres }` dispatch, selected from the
DATABASE_URL scheme. This is prep for the Postgres storage backend landing
in the next commit; zero public-signature changes, and the entire
pre-existing SQLite test suite (moved into db/sqlite.rs verbatim) passes
unmodified, proving current write-queue/WAL behavior is untouched.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ion tooling
Root-cause fix for the chronic write-queue-saturation incidents: SQLite WAL
mode over EFS/NFS has multi-second per-commit lock latency that no amount of
WAL-checkpoint tuning can eliminate. This adds a second Db backend on the
existing shared services-db RDS Postgres instance, wired through the
enum-dispatch facade from the previous commit:
- db/postgres.rs: PostgresBackend using postgres + r2d2_postgres, no
writer-actor/WAL logic (Postgres handles concurrent writers natively via
MVCC/row locking). TLS via postgres-openssl, verified against the bundled
AWS RDS CA chain (certs/rds-global-bundle.pem); DB_TLS_MODE=disable for
local/Docker Postgres dry runs. Pool size via DB_POOL_SIZE (default 10),
5s acquire timeout mapped onto the existing WriteQueueError::Timeout 503
path. writer_healthy() runs a live SELECT 1 with a 2s timeout.
- db/postgres_schema.sql: idempotent schema translated from
migrations/V{1,2,3}, with the derivation-index/log-index columns widened
to BIGINT (the production next_index counter is already within ~42k of
2^31 -- see TODOS.md).
- register_account_auto's next_index allocation uses a single
`UPDATE state ... RETURNING` statement: Postgres's row lock on that one
row serializes concurrent registrations correctly, entirely inside RDS
(no network-filesystem locking involved).
- sqlite_import.rs: reads a full SQLite snapshot (source opened read-only,
never mutated) and writes it into Postgres inside one transaction
(all-or-nothing), with a hard empty-source guard so a freshly-seeded or
partial SQLite file can never wipe out real derivation-index history.
PostgresBackend::connect runs this automatically at construction time
when EVM_MIGRATE_FROM_SQLITE points at an existing file and Postgres is
still empty, holding a pg_advisory_lock for the duration so two ECS tasks
briefly alive during a rolling deploy can't race the migration.
- bin/migrate_sqlite_to_postgres: standalone wrapper around the same
sqlite_import logic for laptop-driven dry runs, with a --verify mode that
re-compares source vs. destination and exits non-zero on any mismatch.
- WriteQueueError message text reworded to backend-neutral phrasing so
Postgres-path incident logs never point responders at writer-actor code
that isn't running.
Cargo.lock/Cargo.toml: add postgres, r2d2_postgres, postgres-openssl,
openssl, tempfile (now a runtime dep -- ships the bundled RDS CA chain to a
temp file at connect time), plus testcontainers dev-deps for the next
commit's parameterized Postgres test suite.
Co-authored-by: Cursor <cursoragent@cursor.com>
…uite Decision D6 from the eng review: the existing per-method Db test suite runs identically against both backends. This adds tests/postgres_backend.rs, gated end-to-end on Docker being available (skips with a printed message, never a failure, so `cargo test` without Docker still runs the full SQLite regression suite untouched): - sqlite_full_crud_cycle_regression_guard / postgres_full_crud_cycle_matches_sqlite: one shared assertion function exercising accounts, deposits, erc20 deposits, token metadata, sweep bookkeeping, and the full webhook-delivery lifecycle, run against a temp-file SQLite Db (always) and a testcontainers postgres:16-alpine Db (when Docker is present) -- the drift guard for the intentionally-duplicated SQL between db/sqlite.rs and db/postgres.rs. - postgres_concurrent_register_account_auto_allocates_distinct_sequential_indices: 16 parallel register_account_auto calls allocate 16 distinct sequential indices, proving Postgres's row lock on the next_index UPDATE ... RETURNING provides the same atomicity the SQLite writer thread used to. - postgres_pool_exhaustion_maps_to_write_queue_error: a pool of size 1 with one transaction held in flight forces a second concurrent write past the 5s acquire timeout (decision D9) and asserts the resulting error downcasts to WriteQueueError::Timeout, so api.rs's existing 503 mapping keeps working unchanged on the Postgres path. - postgres_health_check_reflects_container_availability: writer_healthy() flips false while the container is stopped and a fresh reconnect observes true again once it's back (decision D3 / ALB health-check semantics). - postgres_bootstrap_migration_*: the automatic SQLite->Postgres migration at Db::connect time runs once against a real seeded SQLite snapshot, continues the migrated next_index counter (not a restart-from-0), is a clean no-op on a second connect against the now-populated instance, skips cleanly when EVM_MIGRATE_FROM_SQLITE is unset or points at a missing file, and fails startup loudly (rather than silently booting empty) against an empty/freshly-seeded source -- exercising the empty-source guard through the public Db surface end to end. The container image tag defaults to 16-alpine per the migration plan but is overridable via TEST_POSTGRES_IMAGE_TAG for environments whose registry proxy only has older tags cached. Note for the health-check test: this Docker setup reassigns a container's published host port across a stop/start cycle (unlike a real RDS endpoint across a failover), so it re-resolves the live port via `docker port` rather than trusting testcontainers' creation-time-cached mapping. Co-authored-by: Cursor <cursoragent@cursor.com>
scripts/migrate_data_to_postgres.sh wraps the migrate_sqlite_to_postgres binary into a single pass/fail pipeline for pre-cutover validation: pulls the newest snapshot from a DATA_BACKUP_S3_URI prefix (or takes an already-local wallet.db via --local-wallet-db), folds any -wal sidecar into the main file with a full checkpoint so the binary reads a self-contained snapshot, migrates, verifies, re-runs to confirm the idempotent no-op path, and verifies again -- exiting non-zero on any mismatch so a bad dry run fails loudly instead of silently green-lighting the maintenance-window cutover. Never mutates the original S3 snapshot or takes credentials as arguments (POSTGRES_URL is read from the environment only). Co-authored-by: Cursor <cursoragent@cursor.com>
…eng review Deferred-but-tracked items from the migration plan's "NOT in scope" section: the next_index BIP44 2^31 ceiling guard (counter observed ~42k registrations away in production) and measure-first Postgres write-batching (decision D10). Neither blocks the storage-driver cutover; both are cheapest to act on right after it lands. Co-authored-by: Cursor <cursoragent@cursor.com>
Surfaced by the local dry-run against real production data: SQLite has no fixed-width integer types so read_sqlite_snapshot yields Option<i64>, but the Postgres schema stores last_http_status as INTEGER (int4), which the sync postgres crate refuses to bind an i64 into. Cast at the insert boundary with a clear error if a status code ever doesn't fit (never happens for real HTTP statuses). Also fixes a redundant-closure clippy warning in the same block. Co-authored-by: Cursor <cursoragent@cursor.com>
Caught by the local boot smoke test: the sync `postgres` crate drives each connection via an internally-owned Tokio Runtime and calls Runtime::block_on on it for every request *and* on connection drop/close. Calling into it directly from a thread that already has an async runtime entered panics with "Cannot start a runtime from within a runtime" -- which is exactly what HotWalletService::new() and health() did, since Db::with_pool_size()/writer_healthy() are synchronous and were invoked straight from async fns running on a Tokio worker thread under #[tokio::main]. This was previously masked because: - Every hot-path Db call already goes through Db::blocking() (spawn_blocking), which sidesteps the issue -- confirmed empirically that spawn_blocking threads do NOT carry the "runtime entered" guard that Runtime::block_on checks for. - The startup (Db::with_pool_size) and health (writer_healthy) call sites are the only two places that called Db synchronously without going through spawn_blocking, and both used to be cheap/local-only on the SQLite arm (an atomic status check), so the hazard didn't exist until the Postgres arm made them do real blocking network I/O. - tests/postgres_backend.rs uses plain #[test] (no ambient Tokio runtime), so it never exercised this path either. Fixes: - HotWalletService::new(): construct Db via spawn_blocking. - HotWalletService::health(): check writer_healthy() via spawn_blocking. - HotWalletService::get_block_number(): kept sync (existing signature, still used by a unit test that never touches the DB), added get_block_number_async() for the real /block_number handler to call instead. Verified via a real boot against the migrated production data in Docker Postgres: /health and /block_number now return correctly and the process stays up under RUST_BACKTRACE, instead of aborting on the first request. Co-authored-by: Cursor <cursoragent@cursor.com>
3 tasks
seed_sqlite_source previously only seeded accounts+state, so the bootstrap-migration integration test never proved deposits, erc20_deposits, token_metadata, sweep_meta, sweep_failures, and webhook_deliveries migrate (and stay idempotent) through the actual PostgresBackend::connect path -- only the one-off manual production dry-run covered them. Also tighten token_metadata.decimals to a checked i16 conversion instead of a silent 'as' cast, matching the last_http_status pattern, and apply cargo fmt to the files touched by the Postgres migration work. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
wallet.dblives on an EFS volume (NFS). SQLite WAL mode requires a byte-rangefcntllock round-trip on every commit and every read-transaction begin/end — over NFS this is a well-documented anti-pattern that shows up as multi-second per-write latency, not throughput/CPU pressure.evm_hot_wallet::db: interactive write timed outhas been firing continuously in production while EFSPercentIOLimit, burst credits, CPU, and memory all stayed low. The only real fix without changing the write pattern is to stop putting the DB on a network filesystem.This supersedes the stale PR #6 (
feat/postgres-storage-driver, built against the oldredb-baseddevbranch, 27 commits behind current production) with a rework against the current single-writer-actor + priority-lane + WAL-checkpoint codebase.What changed
src/db/mod.rs):Dbkeeps every existing public method signature unchanged. Internallyenum DbInner { Sqlite(sqlite::Db), Postgres(postgres::PostgresBackend) }, selected from theDATABASE_URLscheme. Zero call-site churn inlib.rs/monitor.rs/sweeper.rs/webhook.rs/api.rs.src/db/sqlite.rs(moved fromsrc/db.rs): unchanged writer-actor/WAL-checkpoint implementation, kept as the rollback path. Existing test suite passes with zero test-file edits (regression guard).src/db/postgres.rs(new): syncpostgres+r2d2_postgres+postgres-openssl(RDS CA-verified TLS). No writer actor / WAL logic needed — Postgres handles concurrent writers natively via MVCC/row locking. Atomicnext_indexallocation viaUPDATE ... RETURNING. Pool-exhaustion errors map onto the existingWriteQueueError::{QueueFull,Timeout}soapi.rs's 503 handling is unchanged.writer_healthy()does a liveSELECT 1with a ~2s timeout.src/sqlite_import.rs+ a startup hook inPostgresBackend::connectmigrate all data from a SQLite file into Postgres inside one transaction, guarded by apg_advisory_lock(safe across ECS rolling-deploy overlap) and an empty-source guard (refuses to migrate a freshly-seeded/empty SQLite file). Triggered byEVM_MIGRATE_FROM_SQLITE+ an empty Postgres destination; idempotent no-op otherwise.src/bin/migrate_sqlite_to_postgres.rs(new binary): laptop-driven dry-run tool wrappingsqlite_import, with a--verifymode that exits non-zero on any source/destination mismatch.scripts/migrate_data_to_postgres.sh(new): one-command wrapper — pulls the latest S3 SQLite snapshot, folds the WAL, runs the migration binary, verifies, safe to re-run.certs/rds-global-bundle.pem: AWS RDS CA bundle for TLS peer verification.webhook_deliveries.last_http_statustype mismatch (Option<i64>from SQLite vs. PostgresINTEGER) during migration.postgrescrate callsRuntime::block_onon its own internally-owned runtime for every request and on connection drop — this panics if invoked directly from a thread that already has an async runtime entered.HotWalletService::new()andhealth()did exactly that; fixed by routing both throughspawn_blocking, matching every otherDbcall which already goes throughDb::blocking()for the same reason.Testing
tests/postgres_backend.rs, viatestcontainers): 9 passed — full CRUD parity with SQLite, concurrentregister_account_autoindex allocation, pool-exhaustion →WriteQueueError::Timeoutmapping, health-check reflects container availability, and 4 bootstrap-migration scenarios (success, idempotency, missing source, empty-source guard).cargo clippy --all-targets: clean.substrate-rail/data/wallet.db*(5.8MB + 4.2MB WAL, never mutated the originals), folded the WAL, migrated into a local Dockerpostgres:13/16-alpine, verified per-table counts /next_index/last_block:<chain>cursors match, re-ran to confirm idempotency, ran the full test suite, and booted the actualevm_hot_walletbinary against the migrated Postgres data —/healthreturnedOK (chains: base, polygon)and/block_number?chain=basereturned the correct migrated cursor (48725027), which is what caught the nested-runtime bug above before it could hit production.Rollout
Base branch is
feat/hang-ports(==fix/invalidate-cache, current production tip) rather thanmain, matching the stacking convention already used by #9/#10 —mainis currently behind the still-open #11.Follow-up substrate-rail PR repoints the
evm_hot_walletdependency pin to this branch and adds theEVM_MIGRATE_ENABLEDentrypoint flag.Closes/supersedes #6.
Made with Cursor