Repository navigation
fix: let control-plane wait out a contended pooler at boot instead of one shot - #835
Merged
Merged
Conversation
… one shot Live integration has been red on main since 2026-08-09 22:16 UTC, four consecutive runs, and the container log names the cause directly: WARNING: database not available at startup: failed to ping database: FATAL: (EMAXCONNSESSION) max clients reached in session mode - max clients are limited to pool_size: 15 The Supabase session-mode pooler is capped at 15 clients shared by CI, agent stacks and the live deployment. Every failure examined, six of six across two weeks, carries that same line, including runs whose reported failing step was the smoke test or the SDK suites rather than the bring-up. What turned a momentary spike into a hard failure is on our side. control-plane called Open exactly once, roughly two seconds into a 145 second healthcheck window, and recorded the result in a static DBReady bool that is never re-evaluated. Losing that single race leaves the process serving a 503 /health for the rest of its life with 143 seconds of the window unused, every dependent container unhealthy, and no path back short of a manual restart. In run 31396685984 the sibling web-e2e job released its own connections 39 seconds before the container was killed, so the pool was already free while the process sat there having given up. OpenWithRetry spends a 75 second budget on the open, sized to fit inside the compose healthcheck's 120 second start_period so a database that is genuinely unreachable still fails the container rather than stretching the boot out. It returns immediately on the new ErrConfig sentinel, because a missing DSN or a transaction-mode pooler will read the same way in seventy five seconds as it does now, and it returns the last error when the budget runs out, so nothing is retried into silence. The startup context for the redis probe is now created after the open, so a slow open cannot expire it and report an available redis as unavailable. This does not fix the over-subscription itself. Fifteen session slots shared by CI, agents and the live stack with no reserved budget for any of them stays open on #631.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe control plane now retries transient database startup failures for 75 seconds. Configuration and incompatible-pooler errors stop immediately. The Redis startup probe context is created after database initialization. ChangesDatabase startup retry flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ControlPlaneServer
participant OpenWithRetry
participant DatabasePooler
participant RedisProbe
ControlPlaneServer->>OpenWithRetry: open database with 75-second budget
OpenWithRetry->>DatabasePooler: attempt connection
DatabasePooler-->>OpenWithRetry: transient failure
OpenWithRetry->>DatabasePooler: retry after 3 seconds
DatabasePooler-->>OpenWithRetry: connected pool or final error
ControlPlaneServer->>RedisProbe: create 10-second probe context
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/control-plane/internal/platform/db/pool.go`:
- Around line 101-120: Update the retry loop around Open to cap each attempt
context timeout at the remaining retry budget using the smaller of
openAttemptTimeout and time.Until(deadline). If no budget remains, return the
last database error immediately; preserve existing configuration and
caller-cancellation handling. Add a test using a server that accepts a
connection but never completes the startup exchange, verifying a short budget
does not overrun.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fe67d22-cf82-4cf3-a7ff-233d089983fe
📒 Files selected for processing (4)
.wolf/buglog.jsonlapps/control-plane/cmd/server/main.goapps/control-plane/internal/platform/db/pool.goapps/control-plane/internal/platform/db/pool_test.go
A host that accepts the TCP connection and then stalls in the Postgres startup exchange blocks for the full ten second attempt timeout regardless of how much budget remains, so a seventy five second budget could run to eighty five and a short one could overrun many times over. The budget exists to fit inside the compose healthcheck window, and overrunning it fails the container just as surely as never retrying at all. Each attempt is now clamped to the remaining budget, with a one second floor so a caller passing a very short budget still gets one honest attempt rather than a context that expires mid-dial and reports a timeout for a connection it never really made. The new test covers the case a refused connection cannot reach: it stands up a listener that accepts and then says nothing, and asserts the call returns in about a second rather than the uncapped ten.
Owner
Author
|
One honest caveat on what that green does and does not prove. In this run live-integration started its stack at 14:55:58 and web-e2e started its own at 14:56:02, so live-integration won the race for the pooler this time and both stacks were healthy in 47 seconds. Nothing in the timings suggests the retry path was exercised at all. The green shows the check passes and confirms the failure is contention-dependent rather than a defect in the code under test; it is not evidence that the retry works. The evidence that the retry works is the tests, both of which fail when the logic is removed:
|
This was referenced Aug 10, 2026
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 10, 2026
…30 minutes (#837) ## What is broken The shared Supavisor session pool is capped at 15 clients and has been refusing control-plane at boot with `EMAXCONNSESSION`. #835 made control-plane wait out a spike instead of giving up after one attempt. That is correct and it is not sufficient: patience does not create a slot. On [run 31402462591](https://github.com/sakibsadmanshajib/hive/actions/runs/31402462591) both full-stack jobs spent their entire 75 second budget being refused, and both failed. ## Root cause A session slot is held for as long as the pgxpool connection that owns it exists, not for as long as it is being used. pgxpool defaults `pool_max_conn_idle_time` to 30 minutes with a `pool_health_check_period` of 1 minute, and `pool_min_conns` to 0. So a consumer that bursts to `pool_max_conns=6` once keeps six of the fifteen slots for the next half hour while running no queries at all, and three mostly idle consumers reach the ceiling between them. Measured on 2026-08-10 with no CI job running at all and one 30 minute old local stack up: six parallel session-mode connections were all refused with `EMAXCONNSESSION`. Zero free slots, no CI involved. The pool was fully squatted by connections nobody was using. ## The change The session DSN now carries `pool_max_conn_idle_time=60s` and `pool_health_check_period=15s`, so an idle connection is released about a minute after its last use instead of half an hour. Worst case release is 60 plus 15 seconds, against 30 minutes plus 1 minute before. Both are pgx-only DSN parameters, so both join `PGX_ONLY_PARAMS` and are stripped from the libpq flavour that psycopg2 and psql consume. Handing a pgx-only parameter to libpq is what crash-looped Open WebUI and took the chat surface to 502 for 50 minutes. Both values are named constants rather than inlined literals, and an explicit value in the input DSN still wins, so a deployment under steady traffic can lengthen the window without a code change. ## Scope limit: this reaches derived environments only This fix lands wherever the DSN comes from `scripts/derive-pooler-dsn.py`, which is CI and the demo box. A developer stack started from a hand-written local `.env` gets nothing from it, and the evidence that motivated this PR was exactly that: a local control-plane, up 30 minutes, squatting slots on the shared pooler. So read this as lowering the base rate, not as closing the problem. A local stack can still exhaust the pool on its own, and the wider consumer inventory stays open on #631 with the permanent consumers recorded in #841. ## What this deliberately does not change `pool_max_conns` for the CI stacks stays at 6. It is a ceiling, not a reservation: pgxpool opens connections lazily from `pool_min_conns=0`, so a booting control-plane asks for one connection regardless of its cap. Lowering the cap frees nothing at boot, which is where the failures happen. The floor is 3 by construction, not by preference. One connection is pinned for the life of the process by `LISTEN tenant_settings_changed` in `tenant/settings/listener.go`, one is held for the duration of a credit reservation by `accounting.PgxAccountLocker`, and that reservation's own ledger and usage writes need at least one more. A cap of 2 would deadlock the reservation path rather than shrink it. `pool_min_conns` stays unset here and is filed as #840. Draining to empty means the first request after an idle window pays a cold TCP, TLS and SCRAM handshake, which is a real latency cost and a separate decision from stopping the squatting. ## Test plan - [x] `TestPoolerDSNCarriesBudgetAndExecMode`, the existing guard against a pgx upgrade silently dropping a DSN parameter, now covers `pool_max_conn_idle_time` and `pool_health_check_period` as well, in unit scope rather than only in the integration lane. Broken deliberately by deleting `pool_max_conn_idle_time=60s` from the DSN under test, which is what a dropped parameter would look like: it fails with `session MaxConnIdleTime: want 1m got 30m0s`, naming pgxpool's default exactly. Restored and green. - [x] `TestSessionPoolReleasesIdleConnections` asserts on the count of live server backends rather than on parsed config, because a config value that never reaches the reaper releases nothing. Against a throwaway Postgres it holds 4 connections, releases them, and observes 0 in 1.32s. - [x] Mutation check on that one too: with the two parameters removed from its DSN it fails after the full 15 second window with `still holding 4`, which is pgxpool's 30 minute default doing exactly what this change fixes. - [x] The package is added to the CI step that has `HIVE_TEST_DB_URL`. Leaving it in the `-short` step would have skipped it silently, which is the never-runs trap of #701 and #708. - [x] `python3 scripts/derive-pooler-dsn.py --self-test`: ok, 33 assertions. - [x] `go vet` and `go test` for `internal/platform/db`: clean. - [ ] Both `Web E2E (full stack)` and `Live integration` green on the same push to main. That is the acceptance evidence and it can only be gathered after merge, because the demo box picks the new DSN up on its own redeploy and every long-lived stack has to restart before its squatted slots come back. No UI surface is touched, so no visual proof applies. Refs #631. Follow-ups filed as #840 and #841. ## On whether #835 should be reverted No. Its `Web E2E` neighbour was not green before it: `Web E2E` failed on runs 31336321712, 31338979151 and 31340632314, all before `0b02f0d`. The one post-merge failure carries the same `EMAXCONNSESSION` line at bring-up as the job beside it, and the retry loop closes its pool on every failed attempt, so it holds no slot while waiting. #835 stops a momentary spike from being permanently fatal; it needs a pool that actually frees up, which is what this PR provides. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved session database connection management by automatically removing idle connections. - Added regular health checks to help maintain reliable database sessions and prevent pool exhaustion. - Preserved compatibility for connection configurations that do not support session-pool settings. - **Tests** - Added integration coverage confirming idle session connections are released as expected. - **Documentation** - Documented recommended session connection-pool timeout and health-check settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Aug 11, 2026
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.
What is broken
Live integration (SDK tests + smoke)has been red on main since 2026-08-09 22:16 UTC, four consecutive runs, most recently run 31396685984. The tracking issue the workflow files automatically, #687, has been open since 2026-08-01 with eleven "Still failing" comments and no owner.The failing step reports only
dependency failed to start: container hive-control-plane-1 is unhealthy. The container log names the actual cause:Six of six failures examined across two weeks carry that same line, including the ones whose reported failing step was the smoke test or the SDK suites rather than the bring-up. The failing step moved on 2026-08-09 because #817 correctly stopped
/healthreporting 200 without a pool; the underlying failure did not change, it just started surfacing at bring-up instead of three steps later.Root cause
Two things compound.
The session-mode pooler is capped at 15 clients shared by CI, agent stacks and the live deployment, and CI has no reserved share of it. A push to main starts two full-stack jobs within one second of each other,
Web E2E (full stack)andLive integration, each booting a control-plane withpool_max_conns=6. In run 31396685984 web-e2e's stack was healthy at 14:16:38 and running Playwright until 14:19:12; live-integration's control-plane pinged at 14:18:01, squarely inside that window, and was refused. That is not the whole story though: run 31358559834 failed the same way with web-e2e skipped entirely, so external consumers can exhaust the pool on their own.The part that is ours to fix is what happens next.
control-planecallsplatformdb.Openexactly once, about two seconds into a 145 second healthcheck window, and records the result in aDBReadybool that is never re-evaluated. Losing that single race leaves the process serving a 503/healthfor the rest of its life, every dependent container unhealthy, and no path back short of a manual restart, with 143 seconds of the health window spent doing nothing. In the failing run web-e2e tore its stack down at 14:19:14 and freed those connections 39 seconds before the container was killed at 14:19:53: the pool was already available while the process sat there having given up.The same defect is worse than a CI annoyance on the demo box. A control-plane restarted during any pool spike comes up permanently degraded, and
restart: unless-stoppeddoes not recycle a merely-unhealthy container.The change
db.OpenWithRetryspends a 75 second budget on the open, sized to fit inside the compose healthcheck's 120 secondstart_periodwith room for the rest of startup, so a database that is genuinely unreachable still fails the container loudly rather than stretching the boot out. Two things it deliberately does not do:db.ErrConfigsentinel. A missing DSN, an unparseable one, or a transaction-mode pooler will read exactly the same in 75 seconds, and retrying one only hides a fatal misconfiguration behind a long silence at boot.The startup context for the redis probe is now created after the database open, so a slow open cannot expire it in advance and turn an available redis into a spurious "redis not available" warning.
What this does not fix
The over-subscription itself. Fifteen session slots shared by CI, agents and the live stack with no reserved budget for any consumer stays open on #631, and the two CI jobs still race each other on every push. This change stops that race from being fatal; it does not stop the race.
Test plan
go test ./apps/control-plane/internal/platform/db/... -count=1 -v— all pass, including two new cases.TestOpenWithRetry_RidesOutATransientRefusalwithreturned after 3.7ms; a budget of 350ms was not spent retrying. The assertion is on elapsed time precisely because a single-attempt version returns in about a millisecond and would satisfy any check on the error value alone.go vet ./apps/control-plane/...clean.go test ./apps/control-plane/... -count=1 -shortclean.Live integrationgreen on this branch. It only runs on push to main or with therun-live-integrationlabel, and it is the check this PR exists to repair, so it should be exercised here before merge.No UI surface is touched, so no visual proof applies.
Summary by CodeRabbit
Bug Fixes
Tests