Repository navigation
fix(lakebase): recycle pool connections and pass through dropped pool options #719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
billmcmillin
wants to merge
3
commits into
databricks:main
Choose a base branch
from
billmcmillin:fix/lakebase-connection-lifetime
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
0ab349f
fix(lakebase): recycle pool connections and pass through dropped pool…
billmcmillin d1ab5bf
Merge branch 'main' into fix/lakebase-connection-lifetime
billmcmillin 888448f
fix(lakebase): use the default lifetime when it overflows Node's timer
billmcmillin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
148 changes: 148 additions & 0 deletions
148
packages/lakebase/src/__tests__/connection-lifetime.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| import pg from "pg"; | ||
| import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; | ||
|
|
||
| import { createLakebasePool } from "../pool"; | ||
| import type { LakebasePoolConfig, Logger } from "../types"; | ||
|
|
||
| // Why connections need a lifetime. Lakebase checks a token only at login and | ||
| // rejects expired ones, so a token is at most an hour old when a connection | ||
| // opens. An open connection keeps working after that, but access granted | ||
| // through Unity Catalog claims in the token stops working 4 hours after the | ||
| // token was minted (measured on a live endpoint: every read on the old | ||
| // connections then fails with 42501 while the connections stay up). So every | ||
| // connection has to close before its token is 4 hours old: | ||
| // | ||
| // token age at login (≤ 1 h) + connection lifetime < 4 h − margin | ||
| // | ||
| // The margin covers a connection checked out past its lifetime (pg-pool only | ||
| // recycles idle clients), login time and clock skew. A lifetime within | ||
| // MAX_SAFE_LIFETIME_MS is safe whatever token the cache hands out. | ||
| const HOUR_MS = 60 * 60_000; | ||
| const TOKEN_LIFETIME_MS = HOUR_MS; | ||
| const CLAIMS_LAPSE_MS = 4 * HOUR_MS; | ||
| const SAFETY_MARGIN_MS = HOUR_MS / 2; | ||
| const MAX_SAFE_LIFETIME_MS = | ||
| CLAIMS_LAPSE_MS - TOKEN_LIFETIME_MS - SAFETY_MARGIN_MS; | ||
|
|
||
| // config.ts falls back to these (PGPORT and PGSSLMODE are read only from the | ||
| // environment), so clear them to keep a developer's shell from changing the | ||
| // result. | ||
| const ENV_KEYS = [ | ||
| "PGHOST", | ||
| "PGPORT", | ||
| "PGDATABASE", | ||
| "PGUSER", | ||
| "PGSSLMODE", | ||
| "LAKEBASE_ENDPOINT", | ||
| "DATABRICKS_CLIENT_ID", | ||
| ] as const; | ||
|
|
||
| beforeEach(() => { | ||
| for (const key of ENV_KEYS) { | ||
| vi.stubEnv(key, undefined); | ||
| } | ||
| }); | ||
|
|
||
| const pools: pg.Pool[] = []; | ||
|
|
||
| afterEach(async () => { | ||
| vi.unstubAllEnvs(); | ||
| await Promise.all(pools.splice(0).map((pool) => pool.end())); | ||
| }); | ||
|
|
||
| // Real factory and pg.Pool, so `pool.options` shows what reached pg. | ||
| // Construction is lazy, so no connection is opened. | ||
| function makePool(config: Partial<LakebasePoolConfig> = {}): pg.Pool { | ||
| const pool = createLakebasePool({ | ||
| host: "ep-test.database.us-east-1.databricks.com", | ||
| database: "databricks_postgres", | ||
| user: "test-user@example.com", | ||
| password: "test-only", | ||
| ...config, | ||
| }); | ||
| pools.push(pool); | ||
| return pool; | ||
| } | ||
|
|
||
| /** The lifetime that reached pg.Pool; pg-pool treats 0 as "never recycle". */ | ||
| function lifetimeMs(pool: pg.Pool): number { | ||
| return (pool.options.maxLifetimeSeconds ?? 0) * 1000; | ||
| } | ||
|
|
||
| describe("connection lifetime", () => { | ||
| test("recycles connections by default, early enough that claims never lapse", () => { | ||
| const pool = makePool(); | ||
|
|
||
| // The exact default is the owners' call; it only has to be safe for a | ||
| // connection that logs in with a nearly expired token. | ||
| expect(lifetimeMs(pool)).toBeGreaterThan(0); | ||
| expect(lifetimeMs(pool)).toBeLessThanOrEqual(MAX_SAFE_LIFETIME_MS); | ||
| }); | ||
| }); | ||
|
|
||
| describe("connection lifetime warnings", () => { | ||
| function makeLogger(): Logger & { warn: ReturnType<typeof vi.fn> } { | ||
| return { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; | ||
| } | ||
|
|
||
| // Policy check: an explicit 0 is a deliberate opt-out, so it's kept, but it | ||
| // should never be silent. | ||
| test("keeps an explicit lifetime of 0 and warns that connections are never recycled", () => { | ||
| const logger = makeLogger(); | ||
| const pool = makePool({ maxLifetimeSeconds: 0, logger }); | ||
|
|
||
| expect(lifetimeMs(pool)).toBe(0); | ||
| expect(logger.warn).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // pg-pool would close every connection as soon as it opens for a negative | ||
| // lifetime, and never recycle for NaN (it turns NaN into 0), so these must | ||
| // not reach pg.Pool. Above 2^31 - 1 ms, Node's setTimeout fires after 1 ms, | ||
| // so pg-pool would close every connection after one use. | ||
| test.each([ | ||
| ["negative", -5], | ||
| ["NaN", Number.NaN], | ||
| ["infinite", Number.POSITIVE_INFINITY], | ||
| ["past Node's timer limit", 2_147_484], | ||
| ["just past Node's timer limit, fractionally", 2_147_483.7], | ||
| ["30 days", 30 * 24 * 60 * 60], | ||
| ])( | ||
| "uses the default lifetime and warns when the lifetime is %s", | ||
| (_name, value) => { | ||
| const logger = makeLogger(); | ||
| const pool = makePool({ maxLifetimeSeconds: value, logger }); | ||
|
|
||
| expect(lifetimeMs(pool)).toBe(lifetimeMs(makePool())); | ||
| expect(logger.warn).toHaveBeenCalled(); | ||
| }, | ||
| ); | ||
|
|
||
| // The longest whole-second lifetime Node's timer can hold is still valid: | ||
| // it's kept (with the too-long warning), not replaced by the default. | ||
| test("keeps the longest lifetime Node's timer can hold", () => { | ||
| const logger = makeLogger(); | ||
| const pool = makePool({ maxLifetimeSeconds: 2_147_483, logger }); | ||
|
|
||
| expect(pool.options.maxLifetimeSeconds).toBe(2_147_483); | ||
| expect(logger.warn).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("warns when the lifetime is too long to recycle connections before claims lapse", () => { | ||
| const logger = makeLogger(); | ||
| makePool({ maxLifetimeSeconds: MAX_SAFE_LIFETIME_MS / 1000 + 1, logger }); | ||
|
|
||
| expect(logger.warn).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("doesn't warn for the default lifetime or a safe one", () => { | ||
| const logger = makeLogger(); | ||
| makePool({ logger }); | ||
| makePool({ maxLifetimeSeconds: 30 * 60, logger }); | ||
| // Still safe, so a warning here would be noise. This pins the threshold to | ||
| // the claims lapse, not to the token's 1-hour expiry. | ||
| makePool({ maxLifetimeSeconds: 2 * 60 * 60, logger }); | ||
| makePool({ maxLifetimeSeconds: MAX_SAFE_LIFETIME_MS / 1000, logger }); | ||
|
|
||
| expect(logger.warn).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, fine as a follow-up:
getLakebaseOrmConfig()(line 174) spreads this config at the top level, but TypeORM only forwards pool options fromextra, and Sequelize has its own pool. So ORM users still won't get recycling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I added #730 and will submit a followup PR asap