Fix the generated client's identity to hold across module instances, in every environment by borisno2 · Pull Request #1548 · OpenSaasAU/stack

borisno2 · 2026-09-13T11:51:43Z

Summary

The generated context's "once per process" client guarantee rested on a module-local clientPromise memo plus globalForClient.opensaasClient, a plain globalThis property written only when NODE_ENV !== 'production'.
A bundler that compiles .opensaas/context.ts into more than one bundle gives each copy its own module scope. Under the old code, two such copies each construct their own client and pool — and in production, where the global was never written, nothing spanned the copies at all. That's backwards: production is where a duplicated pool against the Dev database's socket-multiplexed session, or against a deployment's connection ceiling, costs the most.
getClient() now publishes the client through core's existing processGlobal registry (@opensaas/stack-core/internal) — the same Symbol.for/globalThis mechanism the Engine stamp's origin store and the engine-context face already use — unconditionally, in every environment. The NODE_ENV branch is deleted outright rather than widened.
isRuntimeClient is a structural guard (mirrors unsafe.ts's UnsafeCapableClient shape: orm, sql, raw, transaction) used to verify a value already published under the registry key before adopting it, consistent with how originStore's own is check works.
Added docs/adr/0070-the-generated-clients-identity-is-a-process-wide-registry-entry.md recording the decision and the considered alternatives (a version-namespaced key, a bespoke runtime registry, documenting per-module-instance identity instead).
Reconciled the Dev database's maxConnections doc comment: its headroom was always meant to count distinct processes sharing one sidecar, and this fix is what makes that reading hold for a single app process a bundler happens to duplicate. No change to the constant itself.

Test plan

New test in packages/cli/tests/bundle-client-construction.test.ts: imports two on-disk copies of the generated context.ts under NODE_ENV=production and asserts the pg-pool factory is called exactly once across both — this fails against the old per-module-instance behavior (verified locally by reverting the generator change and confirming the new test fails with "expected 2 to be 1", then restoring the fix and confirming it passes).
Updated packages/cli/src/generator/context.test.ts and its snapshot to assert the new processGlobal-based shape and the absence of NODE_ENV/globalForClient.
pnpm build clean across core, cli, auth, rag, storage, tiptap.
pnpm test green: core (1757 passed), cli (554 passed), auth (509 passed).
pnpm lint clean.
pnpm manypkg fix / pnpm format run.
Changeset added (@opensaas/stack-core + @opensaas/stack-cli, patch).

Closes #1201

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTvKf8g5qdqX9jJaVKSPjv

Generated by Claude Code

…gistry entry The generated context's "once per process" client guarantee rested on a module-local memo plus a globalForClient global written only outside production. A bundler that compiles .opensaas/context.ts into more than one bundle gives each copy its own module scope, so two copies could each construct their own client and pool — and in production, where the global was never written, nothing spanned them at all. getClient() now publishes the client through core's existing processGlobal registry (the same mechanism the Engine stamp's origin store already uses), unconditionally in every environment. Adds ADR-0070 and a probe proving two on-disk copies of the generated module share one client under NODE_ENV=production. Closes #1201 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTvKf8g5qdqX9jJaVKSPjv

changeset-bot · 2026-09-13T11:51:47Z

🦋 Changeset detected

Latest commit: 101d5fa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@opensaas/stack-core	Patch
@opensaas/stack-cli	Patch
@opensaas/stack-auth	Patch
@opensaas/stack-rag	Patch
@opensaas/stack-storage	Patch
@opensaas/stack-tiptap	Patch
@opensaas/stack-ui	Patch
@opensaas/stack-storage-s3	Patch
@opensaas/stack-storage-vercel	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2026-09-13T11:51:48Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
stack-docs	Ready	Preview	Sep 13, 2026 11:52am UTC

chatgpt-codex-connector · 2026-09-13T11:51:48Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

borisno2 · 2026-09-13T11:57:33Z

Code review (medium-high effort, correctness / reuse-simplification-efficiency)

Reviewed the diff against main (context.ts template + snapshot, context.test.ts, bundle-client-construction.test.ts, internal.ts, dev-database.ts doc comment, ADR-0070).

No blocking issues. The fix is well-scoped: processGlobal's existing is-checked registry contract is reused as-is rather than introducing a parallel mechanism, the module-local clientPromise memo is correctly left in place to serialize same-instance racers ahead of the registry, the NODE_ENV gate is removed cleanly (confirmed absent from both the template and the generated snapshot), and the new subprocess test (two copies of the generated module share one client, under NODE_ENV=production) is a real, failing-against-the-old-code integration check rather than a tautology. I traced the failure-recovery path (create() throwing before Reflect.set ever runs, so nothing bad gets published) and the async race in getClient() and didn't find a gap in either.

One non-blocking observation worth a decision, not a fix requirement:

processGlobal('client', isRuntimeClient, () => createClient(config)) in packages/cli/src/generator/context.ts publishes under the bare key client (→ Symbol.for('@opensaas/stack-core/client')), and isRuntimeClient is a structural check only (orm/sql/raw/transaction) — identical across every generated app's client, regardless of config or contract. ADR-0070's "considered options" addresses this shape of risk for two stack versions in one process (rejecting a version-namespaced key because the is check already catches an incompatible shape), but the same reasoning doesn't extend to two different OpenSaaS apps on the same stack version sharing a process — there isRuntimeClient can't tell them apart, since both clients have the exact same shape. If that ever happens (e.g. a custom server combining two generated contexts, or a test runner that disables per-file module isolation and imports two fixture projects' .opensaas/context.ts in one worker), the second app would silently adopt the first app's client/pool — wrong database, not a crash.

This is almost certainly out of scope for what #1201 asked to fix (one app, bundler-duplicated), and may be an acceptable, documented limitation rather than something to change here — but it seems worth either a one-line callout in the ADR's Consequences section (in the spirit of the "costs taken knowingly" pattern this repo already uses elsewhere) or a deliberate "not a concern because X" note, so it's a recorded decision rather than an implicit gap.

Nothing else stood out on reuse/simplification/efficiency — the duplicated isRuntimeClient body between the generated template and the ADR's own code sample is expected, not a real duplication smell, since the generated file has to stay self-contained (only processGlobal is imported from core) and there's no existing runtime type guard in unsafe.ts to import instead (only the UnsafeCapableClient type, no exported predicate).

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01TTvKf8g5qdqX9jJaVKSPjv

Generated by Claude Code

…y key change packages/auth/tests/generated-project.ts reached into the old globalThis key by its bare string name ('opensaasClient') to close and drop the generated bundle's cached client between sequentially-generated projects. That reach-in silently stopped working once the previous commit moved the key to a Symbol.for(processGlobalKey('client')) slot, so a later project's getClient() adopted an earlier, already-closed project's client instead of constructing its own — surfacing in CI as ECONNREFUSED and "database does not exist" in rate-limit-e2e.test.ts. Exports processGlobalKey alongside processGlobal from @opensaas/stack-core/internal so the test harness can address the real key, and updates both the harness's cleanup and the test asserting the slot is empty after close(). Also documents the registry key's known limit (it names the slot, not a particular app/config) in ADR-0070. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTvKf8g5qdqX9jJaVKSPjv

vercel · 2026-09-13T12:02:43Z

Deployment failed for project stack-docs with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit

borisno2 · 2026-09-13T12:02:57Z

Thanks for the review. On the non-blocking observation about processGlobal('client', ...) naming a slot rather than a particular app/config: this turned out to be exactly the shape of a real regression CI caught on this head — packages/auth/tests/generated-project.ts was reaching into the old globalThis.opensaasClient string key to close and drop the cached client between sequentially-generated test projects (each its own config/database). That reach-in silently stopped matching anything once the key moved to Symbol.for(processGlobalKey('client')), so the next generated project's getClient() adopted the previous, already-closed project's client — surfacing as ECONNREFUSED / "database does not exist" in rate-limit-e2e.test.ts.

Fixed in 101d5fa: exported processGlobalKey alongside processGlobal from @opensaas/stack-core/internal so the harness can address the real key, updated its cleanup and the test asserting the slot is empty after close(), and added a "Known limits" section to ADR-0070 stating plainly that the key names the slot, not a particular app — safe today because the only two in-process-multiple-client shapes in this repo are the same config's own module duplication (meant to share) and this harness's strictly-serialised sequence with an explicit close-and-drop between projects (now working again). A future consumer needing multiple different, live-at-once generated bundles in one process would need to widen the key with something config-identifying, which nothing here currently exercises.

Generated by Claude Code

borisno2 · 2026-09-13T12:03:26Z

Note on CI: the Vercel status check on 101d5fa is red with "Deployment rate limited — retry in 24 hours" (docs-preview deployment, account-level Vercel quota). Not caused by this diff — nothing here touches docs/ — and a re-run wouldn't help given the message names a 24h cooldown, so I'm not spending the one re-run on it. Watching for the actual Test GitHub Actions workflow (currently queued on this head) to report.

Generated by Claude Code

github-actions · 2026-09-13T12:22:44Z