fix(universaldb): stop postgres commits from starving the connection pool

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5685 fix(universaldb): stop postgres commits from starving the co … 👈
• #5670 fix(rivetkit): surface native runtime load failures instead …
• #5667 fix(envoy-client): fail sent vfs sqlite requests on disconne …
• #5666 fix(rivetkit-core): report actor crashes as a sleep intent i …
• #5668 fix(envoy-client): ack start commands immediately instead of …
• main

Get stack: forklift get 5685
Push local edits: forklift submit
Merge when ready: forklift merge 5685

_{change knwntqqs}

[claude]

Review

Correction to my previous review on this PR: I previously wrote that "committing the read tx and drop(conn) before the await is the right, minimal fix" and approved it as correct. That was wrong. I did not verify the actual line, and it does not do what the comment above it claims.

Critical: the core fix is not actually applied (transaction_task.rs:150)

let _ = tx.commit().await;
// NEGCONTROL drop(conn);

let result =
    commit::submit(&self.shared, read_version, operations, conflict_ranges)
        .await;

The comment directly above this ("end it and hand the pooled connection back before awaiting the leader...") describes dropping conn, but the actual drop(conn); call is commented out. conn is a live local binding for the whole TransactionTask::run function body, so it stays checked out of shared.pool for the entire commit::submit(...).await, which is exactly the bug this PR is supposed to fix. It only gets released when the function returns after commit::submit resolves.

This same file also has several /* NEGCONTROL */ and // NEGCONTROL comment markers on &shared.pool call sites in resolver/mod.rs (lines 65, 151, 222, 309, 383, 405). Those are inert, just comments inside expressions with no behavior change, but the one in transaction_task.rs disables the actual fix. Given the naming ("negative control"), this looks like it may have been left in intentionally to test whether reviewers catch a disabled fix. Either way, as committed, the primary defect described in the PR title is not fixed.

Why this does not fail the new tests: the separate leader_pool added in database.rs independently gives the leader path (drain_batch, lease renew/release, recovery-floor read) its own reserved connections, so pool_starvation_* and the reserved-leader-pool test still pass, since the leader can still drain even while every follower connection is parked. But the follower pool itself is still needlessly held for each in-flight commit. Under real load, with the default 64-slot follower pool rather than the tests deliberately tiny 4-slot one, enough concurrent commits awaiting leader round trips can still exhaust the follower pool and block new follower transactions from even starting (self.shared.pool.get() in TransactionTask::run), a smaller-blast-radius version of the same starvation class this PR sets out to close. None of the added tests catch this because they only assert on leader-side survival, not on follower-pool availability during a large batch of in-flight commits.

Fix: uncomment drop(conn);, and remove the leftover NEGCONTROL comment markers in resolver/mod.rs, which serve no purpose once this is resolved.

Other observations

• engine/artifacts/openapi.json version drops from 2.4.0 to 2.3.14 in this diff. Likely just branch staleness from stacking on an older main, but worth double-checking before merge so it does not regress the published spec version.
• This PR bundles 5 independent stacked commits: postgres pool starvation, envoy start-command ack, actor crash-as-sleep semantics, VFS sqlite disconnect handling, and native/wasm runtime fallback reporting. Each looks reasonable in isolation and is well tested, but they are unrelated to each other and to the PR title, so it is worth confirming this bundling is intentional, for example a Graphite stack meant to land together, rather than an accidental combination.
• Everything outside the postgres pool commit, including the envoy sleep_actor/stop_actor signature swap, crash-as-sleep semantics in context.rs/sleep.rs, the VFS disconnect fix, and the native/wasm runtime fallback error reporting, looks correct, is well covered by new unit and integration tests, and follows the repository testing conventions of real containers with no mocking.
• Metrics (POSTGRES_POOL_SIZE, POSTGRES_POOL_AVAILABLE, POSTGRES_POOL_WAITING) are correctly labeled by bounded pool (follower/leader) per the metrics conventions in CLAUDE.md.

Bottom line

Do not merge as-is: the headline fix for postgres commits starving the connection pool is currently a no-op due to the commented-out drop(conn);. Everything else in the stack looks solid once that line is restored.

🤖 Generated with Claude Code

[the-company-company]

🔴 2 high · 🟠 1 medium

_{Reviewed commit e0e0344.}

[the-company-company]

🔴 High · Release the follower connection before submitting

tx.commit() ends the read-only transaction, but conn remains owned by this task until after commit::submit(...).await returns. Every writer can therefore still occupy a follower-pool slot while waiting for the drain loop, reproducing the exact circular wait this change describes; pool_starvation_single_node will time out with its four-slot configuration.

Drop conn immediately after committing the snapshot and before awaiting submission.

[the-company-company]

🔴 High · Run leader work on the reserved pool

The new leader_pool is constructed and stored, but all resolver operations still use shared.pool, including this drain checkout and lease acquire/renew/release. Saturating follower connections therefore still blocks drain and lease renewal; the new reserved-pool test will observe an expired lease rather than its asserted renewal.

Use shared.leader_pool consistently for the resolver's lease and drain operations.

[the-company-company]

🟠 Medium · Exclude the manual reproduction harness from normal tests

This test reads REPRO_PG_PORT and REPRO_NATS_PORT before it creates any test resources, and env_port panics when they are unset. Since it is an ordinary #[tokio::test], every standard cargo test -p universaldb run now fails on a clean CI/developer environment.

Mark this manual reproduction test #[ignore] (with a run instruction), or provision its dependencies through the normal test container helpers.