feat(runtime): decouple driver instantiation from binding#151
Conversation
TML-1837 Runtime DX: decouple runtime driver instantiation from connection binding
SummaryToday, runtime drivers (e.g. Postgres) require connection configuration at driver instantiation time (e.g. This ticket proposes separating driver instantiation from connection binding so that connection information is provided only at connect time. This enables:
Motivation / Background
Proposed direction (non-binding)Introduce a runtime driver lifecycle where:
This likely requires one of:
Scope
Acceptance criteria
Non-goals
Notes
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request decouples runtime driver instantiation from connection binding by introducing an unbound driver creation phase and explicit connect-time binding. Drivers are created via descriptor factories without connection configuration, then bound using a new Changes
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
wmadden
changed the base branch from
main
to
tml-1890-runtime-dx-postgres-one-liner-lazy-client-prisma
wmadden
force-pushed
the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
from
af6f58c to
66b91ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/3-targets/7-drivers/postgres/test/driver.basic.test.ts (1)
1-731: 🛠️ Refactor suggestion | 🟠 MajorSplit this test file to keep it under 500 lines.
This file is well over the 500‑line limit; please split by logical describe blocks (e.g.,
driver.basic.streaming.test.ts,driver.basic.connections.test.ts,driver.basic.transactions.test.ts) so each file is self‑contained.As per coding guidelines: "/*.test.ts: Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."
packages/3-targets/7-drivers/postgres/src/postgres-driver.ts (1)
220-234: 🧹 Nitpick | 🔵 TrivialType mismatch:
connect()signature accepts binding but ignores it.The
connect(_binding: PostgresBinding)method accepts a binding parameter but doesn't use it because the connection is already established in the constructor. This implementsSqlDriver<PostgresBinding>but the actual binding happens during construction viacreateBoundDriverFromBinding.This design is intentional per the PR's lifecycle, but consider documenting this in a comment or updating the interface to clarify that "bound" drivers have no-op connect methods.
📝 Optional: Add clarifying comment
- async connect(_binding: PostgresBinding): Promise<void> {} + /** No-op: connection is established at construction time via createBoundDriverFromBinding */ + async connect(_binding: PostgresBinding): Promise<void> {}examples/prisma-orm-demo/src/prisma-next/runtime.ts (2)
20-72:⚠️ Potential issue | 🟠 MajorGuard async singleton initialization and clean up on failure.
With async init, concurrent calls can create multiple clients/drivers, and any error before
runtimeis assigned leaves aClientallocated. Cache the in-flight promise and only publish globals after success; ensure cleanup incatch/finally.🛠️ Proposed fix (promise guard + cleanup)
let runtime: Runtime | undefined; +let runtimePromise: Promise<Runtime> | undefined; let client: Client | undefined; export async function getPrismaNextRuntime(): Promise<Runtime> { - if (!runtime) { - const connectionString = process.env['DATABASE_URL']; - if (!connectionString) { - throw new Error('DATABASE_URL environment variable is required'); - } - - client = new Client({ connectionString }); - - const contract = validateContract<Contract>(contractJson); - - const stack = createSqlExecutionStack({ - target: postgresTarget, - adapter: postgresAdapter, - driver: postgresDriver, - extensionPacks: [], - }); - - const stackInstance = instantiateExecutionStack(stack); - - const context = createExecutionContext({ - contract, - stack, - }); - - const driverDescriptor = stack.driver; - if (!driverDescriptor) { - throw new Error('Driver descriptor missing from execution stack'); - } - - const driver = driverDescriptor.create({ cursor: { disabled: true } }); - await driver.connect({ kind: 'pgClient', client }); - - runtime = createRuntime({ - stackInstance, - context, - driver, - verify: { - mode: 'onFirstUse', - requireMarker: false, - }, - plugins: [ - budgets({ - maxRows: 10_000, - defaultTableRows: 10_000, - tableRows: { user: 10_000 }, - maxLatencyMs: 1_000, - }), - ], - }); - } - return runtime; + if (runtime) { + return runtime; + } + if (!runtimePromise) { + runtimePromise = (async () => { + const connectionString = process.env['DATABASE_URL']; + if (!connectionString) { + throw new Error('DATABASE_URL environment variable is required'); + } + + const nextClient = new Client({ connectionString }); + + try { + const contract = validateContract<Contract>(contractJson); + + const stack = createSqlExecutionStack({ + target: postgresTarget, + adapter: postgresAdapter, + driver: postgresDriver, + extensionPacks: [], + }); + + const stackInstance = instantiateExecutionStack(stack); + + const context = createExecutionContext({ + contract, + stack, + }); + + const driverDescriptor = stack.driver; + if (!driverDescriptor) { + throw new Error('Driver descriptor missing from execution stack'); + } + + const driver = driverDescriptor.create({ cursor: { disabled: true } }); + await driver.connect({ kind: 'pgClient', client: nextClient }); + + const nextRuntime = createRuntime({ + stackInstance, + context, + driver, + verify: { + mode: 'onFirstUse', + requireMarker: false, + }, + plugins: [ + budgets({ + maxRows: 10_000, + defaultTableRows: 10_000, + tableRows: { user: 10_000 }, + maxLatencyMs: 1_000, + }), + ], + }); + + runtime = nextRuntime; + client = nextClient; + return nextRuntime; + } catch (error) { + await nextClient.end().catch(() => undefined); + throw error; + } finally { + runtimePromise = undefined; + } + })(); + } + return runtimePromise; }
38-56:⚠️ Potential issue | 🟠 MajorUse
stackInstance.driverinstead of creating a separate driver viastack.driver.
instantiateExecutionStack()now instantiates the driver as an unbound instance included in the returned stack instance. Remove the separatestack.driver.create()call and usestackInstance.driverdirectly. The driver options (likecursor) must be curried into the descriptor when creating the stack, not passed at instantiation time.🔧 Proposed fix (use stackInstance.driver)
const stackInstance = instantiateExecutionStack(stack); const context = createExecutionContext({ contract, stack, }); - const driverDescriptor = stack.driver; - if (!driverDescriptor) { - throw new Error('Driver descriptor missing from execution stack'); - } - - const driver = driverDescriptor.create({ cursor: { disabled: true } }); + const driver = stackInstance.driver; + if (!driver) { + throw new Error('Relational runtime requires a driver descriptor on the execution stack'); + } + await driver.connect({ kind: 'pgClient', client });Note: Driver-specific options like
cursor: { disabled: true }must be configured by wrapping the driver descriptor'screate()function when the stack is created, not at instantiation time.
packages/1-framework/1-core/runtime/execution-plane/test/stack.types.test-d.ts
Outdated
Show resolved
Hide resolved
packages/2-sql/4-lanes/relational-core/test/ast/driver-types.types.test-d.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
examples/prisma-orm-demo/test/utils/control-client.ts (1)
116-118: 🧹 Nitpick | 🔵 TrivialConsider type-safe pool state tracking.
The cast
(pool as { ended?: boolean }).endedaccesses an undocumented internal property ofpg.Pool. While functional, this relies on implementation details that could change.Alternative: track pool state with a wrapper or boolean flag if this pattern is used elsewhere.
examples/prisma-orm-demo/src/prisma-next/runtime.ts (2)
17-71:⚠️ Potential issue | 🟠 MajorPrevent double initialization after making runtime async.
Because the function now
awaits, two callers can enter beforeruntimeis assigned, leading to duplicate clients/drivers and a lost runtime reference. Add a shared init promise (or similar lock) to guarantee single initialization.🔧 Proposed fix (single-flight initialization)
let runtime: Runtime | undefined; +let runtimePromise: Promise<Runtime> | undefined; let client: Client | undefined; export async function getPrismaNextRuntime(): Promise<Runtime> { - if (!runtime) { - const connectionString = process.env['DATABASE_URL']; - if (!connectionString) { - throw new Error('DATABASE_URL environment variable is required'); - } - - client = new Client({ connectionString }); - - const contract = validateContract<Contract>(contractJson); - - const stack = createSqlExecutionStack({ - target: postgresTarget, - adapter: postgresAdapter, - driver: postgresDriver, - extensionPacks: [], - }); - - const stackInstance = instantiateExecutionStack(stack); - - const context = createExecutionContext({ - contract, - stack, - }); - - const driverDescriptor = stack.driver; - if (!driverDescriptor) { - throw new Error('Driver descriptor missing from execution stack'); - } - - const driver = driverDescriptor.create({ cursor: { disabled: true } }); - await driver.connect({ kind: 'pgClient', client }); - - runtime = createRuntime({ - stackInstance, - context, - driver, - verify: { - mode: 'onFirstUse', - requireMarker: false, - }, - plugins: [ - budgets({ - maxRows: 10_000, - defaultTableRows: 10_000, - tableRows: { user: 10_000 }, - maxLatencyMs: 1_000, - }), - ], - }); - } - return runtime; + if (runtime) { + return runtime; + } + if (!runtimePromise) { + runtimePromise = (async () => { + const connectionString = process.env['DATABASE_URL']; + if (!connectionString) { + throw new Error('DATABASE_URL environment variable is required'); + } + + client = new Client({ connectionString }); + + const contract = validateContract<Contract>(contractJson); + + const stack = createSqlExecutionStack({ + target: postgresTarget, + adapter: postgresAdapter, + driver: postgresDriver, + extensionPacks: [], + }); + + const stackInstance = instantiateExecutionStack(stack); + + const context = createExecutionContext({ + contract, + stack, + }); + + const driverDescriptor = stack.driver; + if (!driverDescriptor) { + throw new Error('Driver descriptor missing from execution stack'); + } + + const driver = driverDescriptor.create({ cursor: { disabled: true } }); + await driver.connect({ kind: 'pgClient', client }); + + runtime = createRuntime({ + stackInstance, + context, + driver, + verify: { + mode: 'onFirstUse', + requireMarker: false, + }, + plugins: [ + budgets({ + maxRows: 10_000, + defaultTableRows: 10_000, + tableRows: { user: 10_000 }, + maxLatencyMs: 1_000, + }), + ], + }); + + return runtime; + })().finally(() => { + runtimePromise = undefined; + }); + } + return runtimePromise; }
38-56:⚠️ Potential issue | 🟠 MajorUse the driver instance from instantiation instead of creating a separate one.
instantiateExecutionStack()creates a driver instance that is stored instackInstance.driver. Creating another viadriverDescriptor.create()leaves the first driver unconnected and unused, causing a resource leak. The cursor options should be routed through the stack instantiation if needed, not applied to a second driver creation.Fix: Use stackInstance.driver
const stackInstance = instantiateExecutionStack(stack); -const driverDescriptor = stack.driver; -if (!driverDescriptor) { - throw new Error('Driver descriptor missing from execution stack'); -} - -const driver = driverDescriptor.create({ cursor: { disabled: true } }); +const driver = stackInstance.driver; +if (!driver) { + throw new Error('Driver missing from execution stack instance'); +} await driver.connect({ kind: 'pgClient', client });packages/3-targets/8-clients/postgres/src/runtime/postgres.ts (1)
153-177:⚠️ Potential issue | 🟠 MajorPrevent concurrent
runtime()calls from creating multiple drivers.
Becauseruntime()is now async, two callers can enter beforeruntimeInstanceis set, leading to multipleinstantiateExecutionStack()+driver.connect()calls and leaked pools. Cache the in-flight initialization promise to dedupe concurrent calls.🔧 Suggested fix (cache initialization promise)
let runtimeInstance: Runtime | undefined; +let runtimePromise: Promise<Runtime> | undefined; return { sql, schema, orm, context, stack, async runtime() { if (runtimeInstance) { return runtimeInstance; } + if (runtimePromise) { + return runtimePromise; + } - const stackInstance = instantiateExecutionStack(stack); - const driver = stackInstance.driver; - if (driver === undefined) { - throw new Error('Relational runtime requires a driver descriptor on the execution stack'); - } - - await driver.connect(binding); - - runtimeInstance = createRuntime({ - stackInstance, - context, - driver, - verify: options.verify ?? { mode: 'onFirstUse', requireMarker: false }, - ...(options.plugins ? { plugins: options.plugins } : {}), - }); - - return runtimeInstance; + runtimePromise = (async () => { + const stackInstance = instantiateExecutionStack(stack); + const driver = stackInstance.driver; + if (driver === undefined) { + throw new Error('Relational runtime requires a driver descriptor on the execution stack'); + } + + await driver.connect(binding); + + runtimeInstance = createRuntime({ + stackInstance, + context, + driver, + verify: options.verify ?? { mode: 'onFirstUse', requireMarker: false }, + ...(options.plugins ? { plugins: options.plugins } : {}), + }); + + return runtimeInstance; + })(); + + try { + return await runtimePromise; + } catch (error) { + runtimePromise = undefined; + throw error; + } }, };
packages/2-sql/4-lanes/relational-core/test/ast/driver-types.test.ts
Outdated
Show resolved
Hide resolved
packages/2-sql/4-lanes/relational-core/test/ast/driver-types.types.test-d.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/integration/test/codecs.test.ts (1)
1-528: 🧹 Nitpick | 🔵 TrivialConsider splitting this test file to comply with the 500-line limit.
This file has 528 lines, exceeding the 500-line guideline. Consider splitting by logical groupings such as:
codecs.timestamp.test.ts(timestamp encoding/decoding tests)codecs.primitives.test.ts(numbers, strings)codecs.nulls.test.ts(null handling)codecs.annotations.test.ts(codec overrides and assignments)As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
packages/3-targets/7-drivers/postgres/test/driver.basic.test.ts (1)
119-121: 🧹 Nitpick | 🔵 TrivialConsider using object matcher for related assertions.
These assertions check related properties of the same array. Per coding guidelines, prefer
toMatchObjectortoEqualfor checking multiple related values.♻️ Optional: Use single assertion
- expect(rows).toHaveLength(4); - expect(rows[0]).toEqual({ id: 1, name: 'a' }); - expect(rows[3]).toEqual({ id: 4, name: 'd' }); + expect(rows).toEqual([ + { id: 1, name: 'a' }, + { id: 2, name: 'b' }, + { id: 3, name: 'c' }, + { id: 4, name: 'd' }, + ]);examples/prisma-orm-demo/src/prisma-next/runtime.ts (2)
20-72:⚠️ Potential issue | 🟠 MajorGuard async initialization to avoid multiple runtimes.
With the new
awaitpath, concurrent callers can all pass the!runtimecheck and initialize multipleClient/driver instances, leaking connections and returning different runtimes. Add an in‑flight promise/mutex so only one initialization runs and all callers await it.
27-56:⚠️ Potential issue | 🟠 MajorEnsure client cleanup if connect/runtime creation fails.
If
driver.connect(...)orcreateRuntime(...)throws, the module-levelclientremains allocated and the half-initialized state is retained. Use a local client and atry/catchto close on failure and only assign to the module-levelclientafter success.🧹 Proposed fix (local client + cleanup)
- client = new Client({ connectionString }); + const nextClient = new Client({ connectionString }); @@ - const driver = driverDescriptor.create({ cursor: { disabled: true } }); - await driver.connect({ kind: 'pgClient', client }); - - runtime = createRuntime({ - stackInstance, - context, - driver, - verify: { - mode: 'onFirstUse', - requireMarker: false, - }, - plugins: [ - budgets({ - maxRows: 10_000, - defaultTableRows: 10_000, - tableRows: { user: 10_000 }, - maxLatencyMs: 1_000, - }), - ], - }); + const driver = driverDescriptor.create({ cursor: { disabled: true } }); + try { + await driver.connect({ kind: 'pgClient', client: nextClient }); + runtime = createRuntime({ + stackInstance, + context, + driver, + verify: { + mode: 'onFirstUse', + requireMarker: false, + }, + plugins: [ + budgets({ + maxRows: 10_000, + defaultTableRows: 10_000, + tableRows: { user: 10_000 }, + maxLatencyMs: 1_000, + }), + ], + }); + client = nextClient; + } catch (err) { + await nextClient.end().catch(() => undefined); + throw err; + }
| ``` | ||
| Group 1 (Core interfaces) ─────────────────────────────────┐ | ||
| │ | ||
| Group 2 (SqlDriver interface) ◄──────────────────────────────┤ | ||
| │ | ||
| Group 3 (Execution stack) ◄─────────────────────────────────┤ | ||
| │ | ||
| Group 4 (Postgres driver) ◄──────────────────────────────────┘ | ||
| │ | ||
| Group 5 (Runtime wiring) ◄──────────────────────────────────┘ | ||
| │ | ||
| Group 6 (Legacy cleanup) ◄──────────────────────────────────┘ | ||
| │ | ||
| Group 7 (Tests — integration) ◄──────────────────────────────┘ | ||
| │ | ||
| Group 8 (Docs/ADR) ◄──────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to fenced code block.
The dependency diagram code block at line 28 is missing a language specifier. While this is a planning document, adding text or plaintext improves rendering consistency.
📝 Suggested fix
-```
+```text
Group 1 (Core interfaces) ─────────────────────────────────┐🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
wmadden
force-pushed
the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
from
3c9a601 to
6fbbcb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/prisma-orm-demo/src/prisma-next/runtime.ts (1)
45-51:⚠️ Potential issue | 🟡 MinorRedundant driver creation—use
stackInstance.driverinstead of manually callingdriverDescriptor.create().
instantiateExecutionStack()already callsstack.driver.create()internally and returns the driver instance instackInstance.driver. Creating another driver viadriverDescriptor.create()results in two separate driver instances.Proposed fix
const stackInstance = instantiateExecutionStack(stack); - - const context = createExecutionContext({ - contract, - stack, - }); - - const driverDescriptor = stack.driver; - if (!driverDescriptor) { + const driver = stackInstance.driver; + if (!driver) { throw new Error('Driver descriptor missing from execution stack'); } - const driver = driverDescriptor.create({ cursor: { disabled: true } }); await driver.connect({ kind: 'pgClient', client }); + const context = createExecutionContext({ + contract, + stack, + });If cursor options are required, configure them in the stack definition rather than creating a separate driver instance.
packages/3-targets/7-drivers/postgres/test/driver.basic.test.ts (1)
119-121: 🧹 Nitpick | 🔵 TrivialConsider using object matcher for related assertions.
Per coding guidelines, prefer
toMatchObjectover multiple individualexpect().toBe()calls when checking 2 or more related values.♻️ Optional: Use toMatchObject
- expect(rows).toHaveLength(4); - expect(rows[0]).toEqual({ id: 1, name: 'a' }); - expect(rows[3]).toEqual({ id: 4, name: 'd' }); + expect(rows).toHaveLength(4); + expect(rows).toMatchObject([ + { id: 1, name: 'a' }, + expect.anything(), + expect.anything(), + { id: 4, name: 'd' }, + ]);Alternatively, just verify the full array:
- expect(rows).toHaveLength(4); - expect(rows[0]).toEqual({ id: 1, name: 'a' }); - expect(rows[3]).toEqual({ id: 4, name: 'd' }); + expect(rows).toEqual([ + { id: 1, name: 'a' }, + { id: 2, name: 'b' }, + { id: 3, name: 'c' }, + { id: 4, name: 'd' }, + ]);
wmadden
force-pushed
the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
from
6fbbcb8 to
d29d78e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
examples/prisma-orm-demo/src/prisma-next/runtime.ts (2)
20-28:⚠️ Potential issue | 🟠 MajorGuard async singleton init to prevent double runtime/client creation.
Asynchronous initialization can race if multiple callers hit this concurrently, creating multiple clients and possibly leaking the first one. Add an in-flight promise gate.
✅ Proposed fix
-let runtime: Runtime | undefined; -let client: Client | undefined; +let runtime: Runtime | undefined; +let runtimePromise: Promise<Runtime> | undefined; +let client: Client | undefined; export async function getPrismaNextRuntime(): Promise<Runtime> { - if (!runtime) { + if (runtime) { + return runtime; + } + if (runtimePromise) { + return runtimePromise; + } + runtimePromise = (async () => { const connectionString = process.env['DATABASE_URL']; if (!connectionString) { throw new Error('DATABASE_URL environment variable is required'); } client = new Client({ connectionString }); @@ runtime = createRuntime({ stackInstance, context, driver, @@ }); - } - return runtime; + return runtime; + })(); + try { + return await runtimePromise; + } catch (error) { + runtimePromise = undefined; + throw error; + } }
45-56:⚠️ Potential issue | 🟠 MajorReuse
stackInstance.driverinstead of creating a second driver instance.
instantiateExecutionStackalready materializes the driver descriptor (line 132 ofstack.ts), storing it asstackInstance.driver. Creating another driver fromstack.driverwastes the first instance and introduces configuration divergence. Replace lines 45–50 to usestackInstance.driverdirectly, matching the pattern inexamples/prisma-next-demo/src/prisma-no-emit/runtime.tsandpackages/3-targets/8-clients/postgres/src/runtime/postgres.ts.examples/prisma-next-demo/src/main-no-emit.ts (1)
52-91:⚠️ Potential issue | 🟠 MajorHandle runtime initialization failures before command dispatch.
getRuntimenow rejects asynchronously but is awaited before thetryblock, so init errors bypass your error handling andruntime.close()isn’t safely guarded. Move initialization into thetryand only close when defined.🔧 Suggested fix
async function main() { const { databaseUrl } = loadAppConfig(); - const runtime = await getRuntime(databaseUrl); - try { + let runtime: Awaited<ReturnType<typeof getRuntime>> | undefined; + try { + runtime = await getRuntime(databaseUrl); if (cmd === 'users') { const limit = args[0] ? Number.parseInt(args[0], 10) : 10; const users = await getUsers(runtime, limit); console.log(JSON.stringify(users, null, 2)); @@ } catch (error) { console.error('Error:', error); process.exit(1); } finally { - await runtime.close(); + if (runtime) { + await runtime.close(); + } } }packages/3-targets/7-drivers/postgres/src/postgres-driver.ts (1)
29-46: 🧹 Nitpick | 🔵 TrivialExtract shared cursor options type to avoid drift.
Both driver option types define the same cursor shape; a shared type keeps them in sync.♻️ Suggested refactor
+type PostgresCursorOptions = { + readonly batchSize?: number; + readonly disabled?: boolean; +}; + export interface PostgresDriverOptions { readonly connect: { client: Client } | { pool: PoolType }; - readonly cursor?: - | { - readonly batchSize?: number; - readonly disabled?: boolean; - } - | undefined; + readonly cursor?: PostgresCursorOptions | undefined; } export interface PostgresDriverCreateOptions { - readonly cursor?: - | { - readonly batchSize?: number; - readonly disabled?: boolean; - } - | undefined; + readonly cursor?: PostgresCursorOptions | undefined; }
...2026-02-15-runtime-dx-decouple-runtime-driver-instantiation-from-connection-binding/tasks.md
Outdated
Show resolved
Hide resolved
...2026-02-15-runtime-dx-decouple-runtime-driver-instantiation-from-connection-binding/tasks.md
Outdated
Show resolved
Hide resolved
...e-runtime-driver-instantiation-from-connection-binding/verifications/final-verification.html
Outdated
Show resolved
Hide resolved
packages/3-targets/7-drivers/postgres/test/driver.errors.test.ts
Outdated
Show resolved
Hide resolved
jkomyno
left a comment
jkomyno
left a comment
There was a problem hiding this comment.
Multi-agent code review — 9 non-blocking inline findings (P2/P3). The P1 race condition in runtime() memoization is tracked separately.
packages/3-targets/7-drivers/postgres/test/driver.basic.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/prisma-next-demo/test/runtime.integration.test.ts (2)
491-509: 🧹 Nitpick | 🔵 TrivialConsolidate assertions using object matchers.
Multiple individual
toHavePropertyandtypeofchecks can be combined intotoMatchObjectassertions for clearer, more maintainable tests.♻️ Proposed refactor using object matchers
expect(rows).toHaveLength(2); -expect(rows[0]).toHaveProperty('id'); -expect(rows[0]).toHaveProperty('email'); -expect(rows[0]).toHaveProperty('posts'); -expect(Array.isArray(rows[0]!.posts)).toBe(true); +expect(rows[0]).toMatchObject({ + id: expect.any(String), + email: expect.any(String), + posts: expect.any(Array), +}); const alice = rows.find((r) => r.email === 'alice@example.com'); expect(alice).toBeDefined(); expect(alice!.posts).toHaveLength(2); -expect(alice!.posts[0]).toHaveProperty('id'); -expect(alice!.posts[0]).toHaveProperty('title'); -expect(alice!.posts[0]).toHaveProperty('createdAt'); -expect(typeof alice!.posts[0]!.id).toBe('string'); -expect(typeof alice!.posts[0]!.title).toBe('string'); +expect(alice!.posts[0]).toMatchObject({ + id: expect.any(String), + title: expect.any(String), + createdAt: expect.any(Date), +}); const bob = rows.find((r) => r.email === 'bob@example.com'); expect(bob).toBeDefined(); -expect(bob!.posts).toHaveLength(1); -expect(bob!.posts[0]!.title).toBe('Third Post'); +expect(bob!.posts).toMatchObject([ + { title: 'Third Post' }, +]);As per coding guidelines: "Prefer object matchers (toMatchObject) over multiple individual expect().toBe() calls when checking 2 or more related values in tests"
124-517: 🧹 Nitpick | 🔵 TrivialConsider splitting test file to stay under 500 lines.
The file is 517 lines, slightly exceeding the 500-line guideline. Consider splitting into logical groups:
runtime.basic.integration.test.ts- streaming, type inference, includeMany testsruntime.budgets.integration.test.ts- budget enforcement testsAs per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
agent-os/specs/2026-02-16-runtime-dx-decouple-driver-instantiation/review-actions.md
Outdated
Show resolved
Hide resolved
wmadden
force-pushed
the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
from
5fe4b6d to
b692d16
Compare
jkomyno
left a comment
jkomyno
left a comment
There was a problem hiding this comment.
@wmadden — thorough review of the PR. The architecture is sound, plane/layer/domain boundaries are clean, and the two-phase driver lifecycle is well-designed. ADR 159 and the test coverage are excellent. A few things need addressing before merge.
Blocking
P1-1: Resource leak on failed runtime initialization
packages/3-targets/8-clients/postgres/src/runtime/postgres.ts:159-177
When driver.connect(binding) succeeds but createRuntime() fails (e.g., codec validation on verify: 'startup'), the promise resets via .catch() and the next runtime() call creates a fresh driver via instantiateExecutionStack. The previous driver — already connected with a live pool — is never closed.
Each retry on a URL binding allocates a new pg.Pool (default 10 connections). In flapping environments, this can exhaust max_connections.
Fix: wrap the post-connect code in try/catch:
runtimePromise = (async () => {
const stackInstance = instantiateExecutionStack(stack);
const driver = stackInstance.driver;
if (driver === undefined) {
throw new Error('Relational runtime requires a driver descriptor on the execution stack');
}
try {
await driver.connect(binding);
return createRuntime({ stackInstance, context, driver, ... });
} catch (e) {
await driver.close().catch(() => {});
throw e;
}
})();P1-2: Driver lifecycle errors use plain Error — no structured codes
packages/3-targets/7-drivers/postgres/src/exports/runtime.ts:22-27, packages/3-targets/8-clients/postgres/src/runtime/binding.ts:25-49
All driver lifecycle errors ('Postgres driver not connected...', 'already connected...', binding validation) are plain Error objects with string messages. The rest of the codebase uses RuntimeErrorEnvelope with code/category/details and SqlQueryError with kind/sqlState. The driver layer — the first thing consumers hit during setup — is the odd one out.
This contradicts the project's "agent-accessible" design goal and forces string-matching for programmatic error handling.
Fix: use runtimeError() or a parallel driverError() with codes like DRIVER.NOT_CONNECTED, DRIVER.ALREADY_CONNECTED, DRIVER.BINDING_INVALID.
P2-1: TOCTOU race in PostgresUnboundDriverImpl state transitions
packages/3-targets/7-drivers/postgres/src/exports/runtime.ts:29-101
Methods check #delegate === null then use #delegate. If close() nullifies #delegate between check and use (concurrent callers), queries go to a closed pool producing confusing errors.
Fix: capture #delegate in a local variable at method entry. Extract a #requireDelegate() helper:
#requireDelegate(): SqlDriver<PostgresBinding> {
const delegate = this.#delegate;
if (delegate === null) throw new Error(USE_BEFORE_CONNECT_MESSAGE);
return delegate;
}P2-2: Reconnection after close() is implicitly supported but untested
runtime.ts:40-45, 54-58
close() resets #delegate = null, so a subsequent connect() succeeds. The error message says "Call close() before reconnecting" — implying reconnection is supported. But no test covers close-then-reconnect. Either add a test or prevent reconnection with a permanent #closed flag.
P2-3: Direct client acquireConnection() concurrency (existing TODO)
postgres-driver.ts:279
The // TODO: This might need to be protected with a mutex is a real concern. Multiple concurrent acquireConnection() calls on a Client-based driver share the same underlying client, leading to nested transaction errors. Promote to a tracked issue or add a mutex.
P2-4: Reliance on undocumented pg internals
postgres-driver.ts:246, 285-300
pool.ended, client._ending, client._connection are undocumented pg internals. These can break on any pg version bump. Maintain explicit internal state flags (e.g., #closed) instead.
P2-5: Driver state not introspectable
SqlDriver interface at driver-types.ts:16-20
ADR 159 defines a clear state machine (Unbound → Connected → Closed) but there's no way to query the current state. An isConnected getter or state property would be a low-effort improvement.
Nits (non-blocking)
-
Credential leakage in runtime error paths (
postgres-driver.ts:326-335): The control-plane driver usesredactDatabaseUrl()but the runtime-plane does not. Connection failure errors frompgmay include the connection string. -
Inconsistent driver creation in integration test utils (
test/integration/test/utils.ts:69-84):instantiateExecutionStack()creates a driver on L69, then a separate driver is created manually on L84. The first is discarded. Use the descriptor-wrapping pattern consistently. -
EXPLAIN SQL concatenation (
postgres-driver.ts:104):EXPLAIN (FORMAT JSON) ${request.sql}uses string interpolation. Currently safe (DSL-generated SQL), but the public API surface could be misused. A comment or validation would help. -
PostgresDriverOptionsappears to be dead code (postgres-driver.ts:29-37): Only used by internal constructors, never imported externally after the refactor. Removeexportor inline. -
Duplicate cursor type shape written out 3 times across
postgres-driver.ts. Extract a sharedCursorOptionstype. -
execute()on unbound driver (runtime.ts:63-77): The manualAsyncIterablewithreturn()andthrow()is 15 lines of ceremony. Anasync function*generator that throws would be 3 lines. -
EXPLAIN_NOT_SUPPORTED_MESSAGEguard (runtime.ts:86-88): Unreachable —createBoundDriverFromBindingonly returns drivers that inheritexplainfromPostgresQueryable. -
Git history: Consider squashing the 64 commits before merge. Only 5 are
feat, 21 arefix(touching the same files), and 7+ are spec-artifact cleanup with zero net diff. The fix-to-feat ratio of 4:1 suggests edge cases were discovered iteratively rather than designed upfront.
Overall the design is well-motivated. The separation of instantiation from binding is clean, the discriminated unions are precise, and the promise memoization with retry is correct. Looking forward to seeing the P1/P2 items addressed.
|
Thanks @jkomyno — I addressed the blocking P1/P2 feedback and pushed the changes. Summary of what changed:
Commits:
Verification:
|
Capture the shaped requirements and full specification for decoupling driver instantiation from connection binding, including fail-fast semantics, typed driver-determined binding, and removal of legacy helper paths.
…tion binding - Add TCreateOptions param to RuntimeDriverDescriptor (default void) - Change create(options?: TCreateOptions) for optional, connection-free instantiation - Add readonly driver to ExecutionStackInstance - Update instantiateExecutionStack to call stack.driver.create() when present - Add unit and type tests for driver-in / driver-out behavior
- SqlDriver<TBinding> interface: connect(binding: TBinding) instead of connect() - Default TBinding = void for drivers without binding - Update mock driver and Postgres driver implementers
…1837 Group 4) - Add PostgresBinding type (url | pgPool | pgClient) - Add PostgresUnboundDriverImpl with connect(binding) lifecycle - postgresRuntimeDriverDescriptor.create() returns unbound driver, no connection args - Use-before-connect fails with clear error - Update SqlDriver<PostgresBinding> for pool/direct impls - Add driver.unbound.test.ts for descriptor create + connect path - Update existing tests to pass binding to connect()
Group 5 (runtime-wiring): Use stackInstance.driver from instantiateExecutionStack, call connect(binding) before createRuntime, assert driver exists. Make runtime() async since connect is async. Update examples to await db.runtime().
…ecycle - prisma-no-emit: make getRuntime async, use create() + connect(binding) - prisma-orm-demo: make getPrismaNextRuntime and createTestRuntime async, use connect(binding) - integration test utils: make createTestRuntime/createTestRuntimeFromClient async, call driver.connect() before createRuntime - update all integration test call sites to await createTestRuntime
…gresDriverFromOptions
…onnect→create flow
Relax runtime driver typing from SqlDriver<void> to SqlDriver<unknown> so clients can use connect(binding) with driver-defined binding types.
…path Drop PostgresDriverOptions from runtime exports and migrate integration test helpers to a local options shape so the descriptor + connect lifecycle remains the primary API signal.
…ckage Use a single canonical PostgresBinding type across client and driver runtime surfaces to reduce drift risk, and remove a duplicate type import in the postgres runtime entrypoint.
Use the TypeScript compilation timeout budget for async config-loader error-path tests so full-suite runs stay reliable under variable I/O and module load timing.
Use the driver created by instantiateExecutionStack() instead of creating a second runtime driver instance in the no-emit runtime setup.
Ensure test runtime setup cleans up created pg pools when driver.connect throws so failures do not leak resources or hang test runs.
Implement return() and throw() on the pre-connect execute iterator so it behaves like a full AsyncIterator while preserving use-before-connect errors.
Add focused mock-based tests for cursor read failure normalization and bound-driver lifecycle edge paths so postgres-driver branch/function coverage meets threshold without loosening limits.
…unctions Set branch and function coverage thresholds to 95% in driver-postgres so enforcement reflects current quality targets while remaining strict.
…failures Use stackInstance.driver in ORM runtime setup and ensure pg pools are closed if driver.connect fails so demo/test helpers do not leak connections.
…setup Ensure seed runtime teardown uses closeTestRuntime so both runtime and pool are closed, preventing leaked connections across integration runs.
…re handling Use SqlDriver<unknown> consistently across sql-runtime interfaces, ensure integration test helper closes pool on connect failure, and update README example to await collected execute results.
…rrent calls Use a promise singleton in postgres runtime initialization so concurrent runtime() callers share one connect/createRuntime flow, and reset the memoized promise on failure so later calls can retry cleanly.
- fix(demo): Driver descriptor missing → Driver missing error message - test(cli): remove duplicate file-not-found test in config-loader - refactor(driver-postgres): remove redundant explain() check; add connect() comments - refactor(postgres): use ifDefined for conditional plugins spread
Switch the postgres client package to root export plus main/module/types metadata and align the test pg Pool mock with current constructor usage.
… options Make PostgresDriverOptions internal and derive the exported PostgresDriverCreateOptions with Omit so cursor option shape cannot drift between constructor and create API.
…me helper Remove the legacy connect-shape adapter in integration test runtime utilities and update call sites to pass PostgresBinding explicitly so helper inputs match the runtime API contract.
If driver.connect succeeds but createRuntime throws, close the connected driver before rejecting so retries cannot leak pools. Also emit structured binding validation errors so callers can handle setup failures without string matching.
…te access Emit structured driver lifecycle errors with stable codes, and avoid TOCTOU on delegate access by capturing the bound driver at method entry. Also expose an optional driver state property and cover close-then-reconnect behavior.
…losed state Prevent concurrent acquireConnection calls from sharing a direct client by serializing leases, and replace reliance on pg internal flags with explicit closed/connected state. Update tests to match the new semantics.
… helper Avoid discarding the driver created during stack instantiation by reusing stackInstance.driver, and curry cursor options into the driver descriptor at stack creation time.
wmadden
force-pushed
the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
from
a4bae38 to
864c9a9
Compare
|
Performed the targeted squash pass after rebasing onto What was done:
Validation after rewrite:
Force-pushed with lease: |
…branches Prevent flaky coverage failures in adapter-postgres by using explicit database operation timeouts on slower tests, and add direct-client branch tests in driver-postgres so branch thresholds are met reliably.
wmadden
deleted the
spec/tml-1837-runtime-dx-decouple-driver-instantiation
branch
Adapt sql-dml.arrays.test.ts to the driver refactoring from PR #151: use createTestRuntimeFromClient (async) instead of createTestRuntime with the old { connect: { client } } shape. Co-authored-by: Cursor <cursoragent@cursor.com>
Adapt sql-dml.arrays.test.ts to the driver refactoring from PR #151: use createTestRuntimeFromClient (async) instead of createTestRuntime with the old { connect: { client } } shape. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
closes TML-1837
Goal / purpose
Make runtime stack instantiation deterministic and env-free by separating driver instantiation from connection binding. Drivers are created unbound as part of
instantiateExecutionStack(), and only become usable afterconnect(binding)at the runtime boundary.Before / After
What changed and why
RuntimeDriverDescriptor.create(options?)is now explicitly non-connection and supports driver-specific create options.ExecutionStackInstancenow includesdriver, soinstantiateExecutionStack()produces a complete stack instance deterministically.SqlDriver.connect(binding)is now generic (TBinding), keeping the shared interface free of Postgres-specific types.postgres().runtime()is now async because it awaitsdriver.connect(binding).Notes / known failures
pnpm test:packagescurrently fails in@prisma-next/integration-kyselywithCommand \"prisma-next\" not found(appears pre-existing).pnpm test:e2ecurrently fails withTypeError: runtime.close is not a function(seeagent-os/specs/2026-02-15-runtime-dx-decouple-runtime-driver-instantiation-from-connection-binding/verifications/final-verification.html).Summary by CodeRabbit
Release Notes
New Features
connect(binding)at runtime.Bug Fixes
Refactor
runtime()calls must be awaited.Documentation
Tests