Repository navigation
fix: release idle session-pooler slots instead of squatting them for 30 minutes - #837
Merged
Merged
Conversation
…30 minutes The shared Supavisor session pool is capped at 15 clients and has been refusing control-plane at boot with EMAXCONNSESSION. PR #835 made control-plane wait out a spike rather than give up after one attempt, which is right and is not enough on its own: patience does not create a slot, and on run 31402462591 both full-stack CI jobs spent their whole 75 second budget being refused. 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, so any 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. Three mostly-idle consumers reach the ceiling between them. Measured on 2026-08-10 with no CI job running and one 30 minute old local stack up: six parallel session-mode connections were all refused with EMAXCONNSESSION, so the pool was fully squatted by connections nobody was using. The session DSN now carries pool_max_conn_idle_time=60s and pool_health_check_period=15s, which turns "pinned for 30 minutes" into "pinned while actually working". Both are pgx-only DSN parameters, so both are added to PGX_ONLY_PARAMS and stripped from the libpq flavour that psycopg2 and psql get. Handing a pgx-only parameter to libpq is what crash-looped Open WebUI and took the chat surface to 502 for 50 minutes. Deliberately not changed: pool_max_conns for CI. It is a ceiling rather than a reservation, so lowering it frees nothing at boot, and the floor is 3 by construction (one connection pinned permanently by LISTEN tenant_settings_changed, one held across a credit reservation by PgxAccountLocker, and at least one for that reservation's own queries), so the plausible-sounding value of 2 would deadlock the reservation path. Test plan: - TestSessionPoolReleasesIdleConnections counts live server backends rather than parsed config, because a config value that never reaches the reaper releases nothing. Against a throwaway Postgres it holds 4, releases them, and observes 0 in 1.32s. - Mutation check: with the two parameters removed from the test's DSN it fails after the full 15 second window with "still holding 4", which is pgxpool's 30 minute default doing exactly what this fixes. - 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 issues #701 and #708. - python3 scripts/derive-pooler-dsn.py --self-test: ok, 33 assertions. - go vet and go test for the package: clean. No UI surface is touched, so no visual proof applies. Refs #631.
|
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSession DSN derivation now adds pgxpool idle cleanup and health-check settings, removes them from libpq DSNs, and includes integration coverage for releasing idle database connections. ChangesSession pool cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 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 |
Owner
Author
|
The stack came up cleanly here: the control-plane got its pool, the healthcheck passed, and the suite ran for 7.3 minutes before reporting That signature predates this branch and predates #835. Run 31340632314, on main on 2026-08-09, failed the same step with the same Nothing in this change can reach a browser navigation. The diff touches the session DSN's connection lifetime parameters and a Go test; the pool releases a connection only after it has been idle past the window, and reopening one is transparent to callers. The two |
This was referenced Aug 10, 2026
TestPoolerDSNCarriesBudgetAndExecMode exists to catch a pgx upgrade silently dropping a DSN parameter, which is the failure mode that would put the session budget back on pgxpool's defaults without anything going red. It did not cover pool_max_conn_idle_time or pool_health_check_period, so those two were guarded only by the integration lane, which needs a live Postgres and does not run on every job. It now asserts MaxConnIdleTime and HealthCheckPeriod alongside MaxConns, and checks that none of the three leaks into the server runtime parameters, where every connection would fail on an unrecognised configuration parameter. Broken on purpose to confirm it can fail: deleting pool_max_conn_idle_time from the DSN under test, which is exactly what a dropped parameter looks like, produces "session MaxConnIdleTime: want 1m got 30m0s". That names pgxpool's 30 minute default, the value this change exists to override. Restored and green.
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 10, 2026
…ly passes on retry (#838) Gate 4 of promoting `Web E2E (full stack)` to a required merge check: drive the measured 7.7 percent flake rate to zero by removing the cause, and make any future retry-pass impossible to miss. ## The two specs Identified from the artifact of CI run [31361681115](https://github.com/sakibsadmanshajib/hive/actions/runs/31361681115), by unzipping the report JSON embedded in the HTML report and reading each test's `outcome` and `results[]`. Not guessed from names. That run collected 34 tests, skipped 6, failed 2 and executed 26. Two of the 26 passed only on a retry, which is the 7.7 percent. Both are in `apps/web-console/tests/e2e/profile-completion.spec.ts`: | Spec | Test | Attempts | | --- | --- | --- | | `profile-completion.spec.ts` | `setup saves profile` | 3 | | `profile-completion.spec.ts` | `profile settings stay reachable while unverified` | 3 | ## Root cause **Category: test infrastructure. Neither is a product race.** I looked for one and did not find it; the browser behaved correctly in every attempt I examined, including the failing ones. Nine spec files copy-paste a `beforeEach` that reseeds the shared Supabase fixtures through a child process. Playwright charges `beforeEach` to the test's own timeout, and the reseed talks to a shared cloud project, so its wall time is set by conditions no spec controls. Step timings for that identical call in that one job: | Test | Attempt | `beforeEach hook` | | --- | --- | --- | | `setup saves profile` | 0 | 19894 ms | | `setup saves profile` | 1 | 19226 ms | | `setup saves profile` | 2 | 2618 ms | | `profile settings stay reachable while unverified` | 1 | **30248 ms** | Against a declared 30 second timeout, every spec in the file was running with a random slice of its budget. The second test's failing attempt spent the entire 30 seconds inside the hook and never opened a page, which is why its error reads `Test timeout of 30000ms exceeded while setting up "context"` and names no product surface at all. A second defect sat underneath. The reseed ran through `execFileSync`, which freezes the worker's event loop, and Playwright's timeout is an ordinary timer that cannot fire while the loop is blocked. `setup saves profile` attempt 1 was therefore reported as **passed at 30194 ms under a 30000 ms timeout**: a test that overran its deadline and went green anyway. That is a camouflage shape in its own right, so it is fixed here too rather than left as a footnote. ## The fix One shared helper, `apps/web-console/tests/e2e/support/fixture-reset.ts`, replacing the copy-pasted block in all nine specs (107 lines deleted). It: 1. runs the reseed as an **awaited async child**, so the worker's event loop keeps turning and Playwright's timeout accounting stays honest, 2. gives the reseed **its own explicit 120 second ceiling**, so a wedged seeder dies with its own message instead of surfacing as a UI timeout against whatever the browser happened to be doing, 3. adds the hook's **measured wall time back onto the deadline** via `testInfo.setTimeout`, so the product always gets exactly the budget the spec author declared. It composes, so a second reseeding hook in the same test adds its own elapsed time on top. Fixing it in the shared helper rather than only in `profile-completion.spec.ts` is deliberate: the other eight files carry the identical defect and are all in scope once this check is required. Nothing here raises `retries`, adds a blind `waitForTimeout`, loosens or deletes an assertion, or skips a spec. I also updated the long comment in `e2e-fixture-seed.mjs` that explicitly reasoned from "runs via execFileSync, fully synchronous". Its ordering guarantee never depended on the event loop being blocked, only on the `await` and on `workers: 1`, and the comment now says so and names the new revisit condition. ## Retries are now visible Today's job log ends with a `list` reporter tail that folds retry-passes into its pass count, so a run that needed two retries reads identically to a clean one. Finding these two meant downloading a 5 MB artifact and unzipping the HTML report's embedded JSON by hand. `flake-reporter.ts` now writes a table of every retry-pass into the GitHub job summary (and stdout locally), and fails the run when there is one at all. **The threshold is zero, and that is argued in the code.** This check is being promoted to required, and a retry-pass is precisely the failure mode that promotion has to survive: it blocks main at random and teaches everyone to press re-run until green, which is the same habit whether the rate is 8 percent or 4. Any non-zero percentage also silently loosens as the suite grows, since one flaky test scores better in a bigger denominator. Retries stay switched on, because they are what produce the second attempt, the trace and the video that identify the flake; what changes is that a run which needed them no longer reports success. Raising it is a one line diff a reviewer can see and argue with, which is why there is no environment variable for it. Not applied to the OWUI nightly config, deliberately: those specs drive real free tier provider latency, and a zero flake gate there would fail nightly for reasons unrelated to this check. ## Red proof Per `docs/TESTING-STANDARD.md`, everything added here was watched failing first. | Break | Result | | --- | --- | | `gateTripped = false` in `buildFlakeReport` | RED, 2 of 6 unit tests, both threshold cases | | `executed = entries.length` (skipped back in the denominator) | RED, 2 of 6 unit tests, the 7.7 percent case and the skipped-exclusion case | | Temporary spec that fails on attempt 0 and passes on attempt 1, run through the reporter | RED, `playwright` exit code **1**, summary named the spec and "Attempts 2" | | Same spec filtered to its always-passing test | GREEN, exit code **0**, summary read "No test needed a retry" | The unit tests cover the pure `buildFlakeReport`; the reporter's mapping from Playwright's `Suite` onto it is what the two end to end runs above prove, so neither half is only asserted against a hand copy of itself. ## Before and after, 10 runs each Both arms ran against the same booted control-plane and console, the same live Supabase project and the same seeder, one attempt per trial (`--retries=0`). Only the spec changed between arms, restored with `git stash`. **Natural seeder latency** (measured 9.6 s to 20.3 s on this box, no injection): | Test | Before | After | | --- | --- | --- | | `setup saves profile` | **0/10** | **10/10** | | `profile settings stay reachable while unverified` | 10/10 | 9/10 | The second test did not reproduce in that window: this box's seeder happened to stay fast enough for it. So I reran it with the hook latency pinned to what the failing CI attempt actually showed (about 30 s total), which is the condition that broke it in CI: | Test | Before | After | | --- | --- | --- | | `profile settings stay reachable while unverified`, CI hook latency | **0/10** | **9/10** | The one remaining failure in each after arm was an upstream Supabase problem, not the budget: a `Could not query the database for the schema cache` retry in one, and a sign-in exceeding its own 25 s timeout on a heavily loaded box in the other. Both are now reported as what they are, a seeder failure and a sign-in timeout, rather than as an unexplained UI timeout. That is the fix working as intended. The mechanism is also visible directly in the run output: the same spec on the same box reports `Test timeout of 30000ms exceeded` before and `Test timeout of 43103ms exceeded` after, the difference being the 13103 ms the hook actually took. ## Residual flake rate, and what it means for promoting this check **Roughly 10 percent per trial on one test. Not zero, and it should not be read as zero.** Across the two after arms, `profile settings stay reachable while unverified` failed once in each set of ten, so 1/10 and 1/10. Neither failure was the timeout budget this PR fixes: | Arm | Failure | Attribution | | --- | --- | --- | | natural latency | `accounts upsert failed: Could not query the database for the schema cache` | Supabase PostgREST, surfaced by the seeder | | CI hook latency | `page.waitForURL: Timeout 25000ms exceeded` in `signIn` | sign-in exceeded the helper's own 25 s cap on a loaded box | The attribution is sound in mechanism, both failures carry an explicit upstream error rather than an inferred one, but the evidence for each is a single occurrence and no artefact is linked, so treat the mechanism as established and the rate as a rough estimate rather than a measured constant. **The consequence matters for the promotion decision and should not be discovered afterwards.** With `retries: 2` and a threshold of zero, an upstream hiccup of this kind consumes a retry, and if it recurs on the retry the job goes red; if the retry rescues it, the run becomes a retry-pass, which this PR now deliberately fails. Either way a transient Supabase or sign-in stall becomes a red required check once `Web E2E (full stack)` is required. That is the intended behaviour, since the alternative is exactly the re-run-until-green habit this work exists to prevent, but it means promotion should account for a non-zero rate of reds that no code change in this repository can remove. Two things reduce it, neither in scope here: #837 makes the pooler less contended, which shortens the seeder and narrows the window; and the 25 second cap inside `signIn` is a hardcoded local timeout that does not scale with load, which is worth revisiting separately. ## Review follow-ups (second commit) **The gate had an escape hatch, now closed.** `npx playwright test --reporter=list` replaces the configured reporters outright. Under that flag the custom reporter never ran and the run exited 0 on a retry-pass, so one command line argument disabled the whole gate. `failOnFlakyTests: true` is now set alongside the reporter and survives the override. Both layers are kept on purpose: the built-in flag gives an exit code and nothing to act on, the reporter is what names the test and its attempt count. | Break | Result | | --- | --- | | retry-pass, `--reporter=list`, flag present | exit **1** | | retry-pass, `--reporter=list`, flag removed | exit **0**, and the reporter never ran at all | | clean run, `--reporter=list`, flag present | exit **0** | | retry-pass, configured reporters | exit **1**, summary table names one spec | | reseed helper driven down its failure path | logs elapsed and relays the child's error, product budget kept at 30000 | **`live-auth.ts` no longer blocks the event loop.** It is the sanctioned auth helper and its `reauthenticate` is built for mid-test use, so the first mid-test caller would have silently disabled every timeout in that file, which is precisely the defect this PR removes from the reseed. It has no callers yet, so the conversion touched nothing else. **`owui.setup.ts` keeps `execFileSync`, deliberately.** It runs once inside a setup project rather than mid-test, so the only deadline it can eat is its own, and `stdio: "inherit"` streams the installer's progress into the job log live where a buffered async child would withhold it until exit. The reasoning is now a comment in the file so the next reader does not have to rediscover it, with the condition that would change the answer. **Three corrections.** The flake table repeated the spec path in both columns, because `titlePath()` carries a leading empty root and slicing before filtering kept the file; it filters first now and the comment matches the code. `fixture-reset.ts` promised a "reseed exceeded" message it never emitted, so a timeout kill produces one instead of a bare signal. Hook elapsed time was logged only on failure, which is backwards for the one number that diagnoses this class, so it prints on every reseed alongside the preserved product budget. ## Scope added after review, third commit Flagged explicitly because it was not part of what was approved, and an earlier approval should not be stretched over it silently. An independent audit of the `Web E2E` failures on main traced them to this branch's fix, with a head to head control run on the same live Supabase three minutes apart: all three `auth-shell.spec.ts` tests green on first attempt at 0 percent flake here, against two of them failing on main at 36.3 s and 32.6 s under a 30000 ms timeout. A test overrunning its own declared timeout and being recorded at all is the event loop freeze this PR removes, so that is a direct confirmation of the mechanism from an independent run. That audit surfaced one more test of the same family, and this commit **deletes** it: `workspace switcher persists selected account` in `auth-shell.spec.ts`. Why it went rather than being repaired: - **Its core assertion cannot fail.** It called `switcher.selectOption(secondValue)` and then asserted `expect(select).toHaveValue(secondValue)` on the same element. `selectOption` sets that value client side, so the assertion was already true when it was reached, before any navigation. It read as a persistence check and was not one. That is camouflage shape 3 in `docs/TESTING-STANDARD.md`. - **It was a strictly weaker duplicate.** `console-workspace-switch.spec.ts`, `switching workspace takes effect and survives a reload`, switches by workspace name, calls `page.reload()`, and asserts the value the server rendered from the cookie the switch handler set, in both directions. **That is where the coverage lives, and it is unchanged by this PR.** It passed in 14.7 s in the very run where the deleted test failed, so deleting the weaker one loses nothing. - Two smaller faults came with it: it skipped itself whenever the account had fewer than two workspaces, so on a single workspace account it was silently absent, and it hand rolled the switch instead of calling the shared `tests/e2e/support/workspace-switch.ts`. The alternative was to rewrite it to call `switchToWorkspace`, reload, and assert the server rendered value. That would have made it a second copy of the test named above, so deletion was the smaller and honest change. A comment in `auth-shell.spec.ts` records what was removed, why, and which test retains the coverage, so nobody rediscovers the gap and writes the weak version again. `auth-shell.spec.ts` goes from three tests to two, confirmed with `--list`. Every import it had is still used by the invitation test that remains. ## Separate finding, not fixed here `profile-completion.spec.ts` contains a wait that never waits, the same shape as #826: ```ts await page.getByRole("button", { name: "Save billing details" }).click(); await page.waitForURL("**/console/settings/billing"); // already on this URL ``` Measured in the CI artifact, that step resolves in **0 ms**, and the following `toHaveValue("Acme Labs LLC")` then passes in 12 ms by reading the value the test itself typed into the unsubmitted input. `billing settings save partial business profile` therefore cannot distinguish a saved billing profile from a silently discarded one. It was not flaky and is not this PR's cause, so it is filed separately rather than changed here, because fixing the assertion honestly requires first establishing whether the save actually persists. ## Scope note for reviewers `console-budgets.spec.ts` and `console-workspace-switch.spec.ts` are also touched by #829. The overlap is only the `beforeEach` block at the top of each file, well away from the test bodies #829 edits, so whichever merges second should resolve trivially. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This was referenced Aug 11, 2026
Backlog inventory 2026-08-11: 13 issues were fixed and left open, and the backlog is 281 not 90
#853
Closed
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
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 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_timeto 30 minutes with apool_health_check_periodof 1 minute, andpool_min_connsto 0. So a consumer that bursts topool_max_conns=6once 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=60sandpool_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_PARAMSand 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.envgets 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_connsfor 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_changedintenant/settings/listener.go, one is held for the duration of a credit reservation byaccounting.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_connsstays 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
TestPoolerDSNCarriesBudgetAndExecMode, the existing guard against a pgx upgrade silently dropping a DSN parameter, now coverspool_max_conn_idle_timeandpool_health_check_periodas well, in unit scope rather than only in the integration lane. Broken deliberately by deletingpool_max_conn_idle_time=60sfrom the DSN under test, which is what a dropped parameter would look like: it fails withsession MaxConnIdleTime: want 1m got 30m0s, naming pgxpool's default exactly. Restored and green.TestSessionPoolReleasesIdleConnectionsasserts 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.still holding 4, which is pgxpool's 30 minute default doing exactly what this change fixes.HIVE_TEST_DB_URL. Leaving it in the-shortstep would have skipped it silently, which is the never-runs trap of LiteLLM config sync queries a column that does not exist, so the provider catalog can never reach LiteLLM #701 and Tests that never execute in CI: phase-19 tenant-isolation suite, edge-api DB tests, and 4 more integration tests share #701's failure shape #708.python3 scripts/derive-pooler-dsn.py --self-test: ok, 33 assertions.go vetandgo testforinternal/platform/db: clean.Web E2E (full stack)andLive integrationgreen 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 E2Eneighbour was not green before it:Web E2Efailed on runs 31336321712, 31338979151 and 31340632314, all before0b02f0d. The one post-merge failure carries the sameEMAXCONNSESSIONline 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.Summary by CodeRabbit
Bug Fixes
Tests
Documentation