Repository navigation
fix(lakebase): recycle pool connections and pass through dropped pool options - #719
Open
billmcmillin wants to merge 1 commit into
Open
billmcmillin wants to merge 1 commit into
billmcmillin wants to merge 1 commit into
Conversation
… options getLakebasePgConfig() built the pg.Pool config from a fixed list of 11 keys, so maxLifetimeSeconds, application_name, keepAlive, query_timeout and options were silently dropped. Without a lifetime, pool connections were never recycled. Access granted through Unity Catalog claims in a Lakebase OAuth token stops working 4 hours after the token is minted while the connection stays open, so a busy pool kept failing with "permission denied" until the app restarted. - Pass the five options through to pg.Pool, keeping the explicit key list. - Default maxLifetimeSeconds to 1800 (30 minutes); an explicit 0 is kept. - Warn when maxLifetimeSeconds is 0 or above 9000 (2.5 hours): the 4-hour lapse, less up to an hour of token age at login, less a 30-minute margin. - Ignore a negative or non-finite maxLifetimeSeconds, with a warning, and use the default. pg-pool would otherwise close every connection as soon as it opens (negative) or never recycle (NaN). - Document maxLifetimeSeconds, including that held clients are only recycled on release, and regenerate the API docs. - Add unit tests for the default, the warnings and the pass-through. - Add tools/lakebase-soak.ts, a soak harness that runs a real pool against a sandbox endpoint and checks connection age, token age at login, application_name and table access. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Bill McMillin <bill.mcmillin@databricks.com>
This branch has not been deployed
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
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.
Fixes #718.
Summary
createLakebasePool()drops mostpgpool options, includingmaxLifetimeSeconds, so pool connections are never recycled. Access granted through Unity Catalog claims in a Lakebase OAuth token stops working 4 hours after the token is minted, while the connection stays open. A busy pool keeps its connections past that point, and from then on every query that relies on that access fails with42501 permission denied, until the app restarts.This PR passes the dropped options through and recycles connections every 30 minutes by default.
Root cause
getLakebasePgConfig()builds thepg.Poolconfig from a fixed list of 11 keys:LakebasePoolConfigextendspg'sPoolConfig, so any other option type-checks and is then silently dropped. Two of the consequences:maxLifetimeSecondsis always pg-pool's default of 0 (never recycle). In a live soak onmain, pool connections were 4.3 hours old by the end of the run.application_name,keepAlive,query_timeoutandoptionsnever reach the server. Onmain, every soak connection reported an emptyapplication_name.Lakebase checks the token only at login, so connections on native Postgres grants keep working however old they get. Claims access doesn't: on
main, it lapsed 240.5 minutes after the token was minted, on every connection, while liveness queries on the same connections kept succeeding.Changes
packages/lakebase/src/pool-config.ts(the fix)maxLifetimeSeconds,application_name,keepAlive,query_timeoutandoptionsthrough topg.Pool.workspaceClient,endpoint,loggerandtelemetry, which must not reachpg. Andhost,maxand the other pool options are normalised byparsePoolConfig()first, which a spread could overwrite. The cost is that the nextpgoption a caller needs has to be added to the list too.maxLifetimeSecondsto 1800 (30 minutes). The default only has to be well under the safe limit below. Recycling is cheap: a recycled connection logs in again with the cached token, so it doesn't mint a new one. The value is resolved with??, so an explicit0(never recycle) is still honoured.resolveMaxLifetimeSeconds()logs a warning when the lifetime is0, or above 9000 seconds (2.5 hours), and returns it unchanged; it doesn't cap. The limit is the 4-hour lapse, less up to an hour of token age at login (Lakebase rejects expired tokens, so a token is at most an hour old when a connection opens), less a 30-minute margin for a client held past its lifetime, login time and clock skew.maxLifetimeSecondslogs a warning and falls back to the default. Otherwise pg-pool would close every connection as soon as it opens (negative), or never recycle (NaN, which it turns into 0). Before this PR the value was dropped, so apps passing one work today. That's why this warns rather than throws.packages/lakebase/src/types.tsmaxLifetimeSecondsonLakebasePoolConfig. It's inherited fromPoolConfig; it's redeclared only to carry the docs.docs/docs/api/appkit/Interface.LakebasePoolConfig.mdpnpm docs:buildfrom the doc comment above.packages/lakebase/src/__tests__/connection-lifetime.test.ts(new)createLakebasePool()and a static password, then readspool.optionsto see what reachedpg.Pool. Construction is lazy, so no connection is opened and no credentials are requested.PG*,LAKEBASE_ENDPOINTandDATABRICKS_CLIENT_IDenvironment variables before each test, so a developer's shell can't change the results.packages/lakebase/src/__tests__/pool-timeouts.test.tstest.eachblock checks each of the five options reachespool.options. It follows the pattern of the timeout test already in this file.tools/lakebase-soak.ts(new, not published, not run in CI)application_nameand table access. It imports the package from source, so the same command comparesmainwith a branch.application_name, and optionally reads a probe table. Slots are added on a ramp, so connections open throughout a token's life. It disables the idle timeout, so connections live long enough to test recycling, and it holds one connection checked out for the whole run, to show the held-client limit.events.jsonlandsummary.json, and exits 0 only if every check passes:application_nameis on every connection;--claims-tablemints tokens with READ_ONLY claims for a UC table.--ttl-secondsmints shorter-lived claims tokens.--probe-tablenames the table to read.tools/.token-refresh.tsis unchanged, so the pool's token mint rate doesn't change.Testing
New tests in
connection-lifetime.test.ts, plus a pass-through block inpool-timeouts.test.tsthat follows that file's existing pattern. 11 of the 12 fail onmainand pass with the fix:main0is kept and warnsNaNor infinite value uses the default and warnsapplication_name,keepAlive,query_timeout,options,maxLifetimeSecondsreachpool.optionspnpm testpasses apart from onemock-workspace-clienttest that already fails onmain.pnpm build,pnpm docs:build,pnpm checkandpnpm -r typecheckpass.Live 260-minute soaks on a sandbox Lakebase Autoscaling project (1 CU), as a service principal whose only access to the probe table comes through Unity Catalog, with tokens that request READ_ONLY claims for it:
mainapplication_name42501from minute 240.5, on every connection42501from minute 240.542501from minute 240.5, as documentedThe lapse isn't tied to the token's expiry. With one-hour tokens, reads kept working for 15 minutes past expiry, and with 5-minute tokens for 29 minutes, so there's no shorter way to test it than a run of more than 4 hours.
Notes
query_timeoutandkeepAlive, now take effect where they were ignored before.maxLifetimeSeconds.@databricks/lakebaselogs only errors by default, so there they appear only when a logger withwarnis configured. Each pool warns once when it's created, so AppKit's per-user pools repeat the warning per user.getLakebaseOrmConfig()now includesmaxLifetimeSeconds. Whether TypeORM and Sequelize pass it to their own pools hasn't been checked.createLakebasePoolcan't request Unity Catalog claims today. This fix is a prerequisite for adding that, but the feature itself is separate.This pull request and its description were written by Isaac.