-
Notifications
You must be signed in to change notification settings - Fork 0
fix: stabilize local auth session exchange #83
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { shouldUsePerRequestDatabase } from './database'; | ||
|
|
||
| describe('shouldUsePerRequestDatabase', () => { | ||
| it('uses a short-lived database client for localhost requests', () => { | ||
| expect( | ||
| shouldUsePerRequestDatabase({ | ||
| env: { APP_BASE_URL: 'https://opencalendly.com' }, | ||
| req: { url: 'http://localhost:8787/v0/auth/me' }, | ||
| }), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('uses a short-lived database client for localhost app configuration', () => { | ||
| expect( | ||
| shouldUsePerRequestDatabase({ | ||
| env: { APP_BASE_URL: 'http://localhost:3000' }, | ||
| }), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('keeps runtime database reuse enabled for non-local environments', () => { | ||
| expect( | ||
| shouldUsePerRequestDatabase({ | ||
| env: { APP_BASE_URL: 'https://opencalendly.com' }, | ||
| req: { url: 'https://api.opencalendly.com/v0/auth/me' }, | ||
| }), | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,39 @@ | ||
| import { createRuntimeDb } from '@opencalendly/db'; | ||
| import { createDb, createRuntimeDb } from '@opencalendly/db'; | ||
|
|
||
| import { jsonError } from './core'; | ||
| import { isNeonDatabaseUrl, resolveConnectionString } from './env'; | ||
| import type { ContextLike, Database } from './types'; | ||
|
|
||
| const LOCAL_DATABASE_HOSTS = new Set(['localhost', '127.0.0.1', '::1']); | ||
|
|
||
| const normalizeHostname = (value: string): string => { | ||
| return value.trim().toLowerCase().replace(/^\[(.*)\]$/, '$1'); | ||
| }; | ||
|
|
||
| export const shouldUsePerRequestDatabase = (context: Pick<ContextLike, 'env'> & { req?: { url: string } }): boolean => { | ||
| if (context.req?.url) { | ||
| try { | ||
| const requestUrl = new URL(context.req.url); | ||
| if (LOCAL_DATABASE_HOSTS.has(normalizeHostname(requestUrl.hostname))) { | ||
| return true; | ||
| } | ||
| } catch { | ||
| // Ignore malformed request URLs and fall back to APP_BASE_URL detection. | ||
| } | ||
| } | ||
|
|
||
| const configuredAppUrl = context.env.APP_BASE_URL?.trim(); | ||
| if (!configuredAppUrl) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return LOCAL_DATABASE_HOSTS.has(normalizeHostname(new URL(configuredAppUrl).hostname)); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| export const isUniqueViolation = (error: unknown, constraint?: string): boolean => { | ||
| if (!error || typeof error !== 'object') { | ||
| return false; | ||
|
|
@@ -33,6 +63,20 @@ export const withDatabase = async ( | |
| return jsonError(context, 500, 'DATABASE_URL must point to Neon Postgres (*.neon.tech).'); | ||
| } | ||
|
|
||
| if (shouldUsePerRequestDatabase(context)) { | ||
| // Wrangler/workerd local dev can hang retained pg pools after auth/session requests. | ||
| // Use a short-lived client for localhost flows and keep pooled reuse for deployed runtimes. | ||
| const { client, db } = createDb(connection.connectionString, { | ||
| enforceNeon: connection.source === 'database_url', | ||
| }); | ||
| await client.connect(); | ||
| try { | ||
| return await handler(db); | ||
| } finally { | ||
| await client.end().catch(() => undefined); | ||
| } | ||
| } | ||
|
Comment on lines
+66
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n--- withDatabase localhost branch ---\n'
sed -n '49,84p' apps/api/src/server/database.ts
printf '\n--- Database type definition ---\n'
rg -n -C2 'type Database|export type Database|interface Database' apps/api/src/server
printf '\n--- createDb / createRuntimeDb return sites ---\n'
sed -n '56,105p' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 3708 🏁 Script executed: # Find the createRuntimeDb definition
rg -n -A10 'export.*createRuntimeDb|function createRuntimeDb' packages/db/srcRepository: rohan-patnaik/opencalendly Length of output: 732 🏁 Script executed: # Check for TypeScript errors or build logs
find . -name "*.ts" -path "*/api/src/server/*" -exec wc -l {} \; | head -20
# Check if there are actual build outputs or CI configs
fd -e 'build.log' -e 'tsconfig.json' -e '.eslintrc'Repository: rohan-patnaik/opencalendly Length of output: 1453 🏁 Script executed: # Get the complete createRuntimeDb definition
sed -n '107,135p' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 917 🏁 Script executed: # Also check what createRuntimeDb actually returns
rg -A20 'export const createRuntimeDb' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 592 🏁 Script executed: # Verify the exact type mismatch by checking the Drizzle return signatures
rg -B5 -A15 'const drizzle' packages/db/src/client.ts | head -60Repository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Check if there are any type assertions or as-casts in the database.ts handler
sed -n '66,78p' apps/api/src/server/database.ts | cat -nRepository: rohan-patnaik/opencalendly Length of output: 670 🏁 Script executed: # Check the full context around the handler call to see if there's a type cast
sed -n '60,85p' apps/api/src/server/database.tsRepository: rohan-patnaik/opencalendly Length of output: 933 🏁 Script executed: # Check if there's a TypeScript config and any build status
find . -name "tsconfig.json" | head -5Repository: rohan-patnaik/opencalendly Length of output: 195 🏁 Script executed: # Look for any type assertions or workarounds in the handler signature
rg -n 'as Database|as any|type Database' apps/api/src/server/database.tsRepository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Check what the Drizzle return type actually looks like for both drivers
rg -B3 -A3 'drizzle\(' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 524 🏁 Script executed: # Check package.json for build/type-check scripts and versions
cat apps/api/package.json | grep -A5 '"scripts"\|"dependencies"\|"drizzle"' | head -30Repository: rohan-patnaik/opencalendly Length of output: 587 🏁 Script executed: # Check if the types are actually different by examining the schema export
rg -n 'export.*schema' packages/db/src/client.ts | head -5Repository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Verify if drizzle returns a compatible type regardless of Client vs Pool
rg -B2 -A2 'schema.*=' packages/db/src/client.ts | head -20Repository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Check the exact TypeScript configuration to understand strict mode
cat apps/api/tsconfig.jsonRepository: rohan-patnaik/opencalendly Length of output: 305 🏁 Script executed: # Look for import of Database type in database.ts to understand the full context
head -30 apps/api/src/server/database.tsRepository: rohan-patnaik/opencalendly Length of output: 1028 🏁 Script executed: # Check if there are any test files or build outputs that show actual errors
find . -name "*.test.ts" -path "*/server/*" | head -3Repository: rohan-patnaik/opencalendly Length of output: 185 🏁 Script executed: # Check if there's any CI config or build log showing the actual error
find . -name "*.yml" -o -name "*.yaml" | grep -i github | head -5Repository: rohan-patnaik/opencalendly Length of output: 227 🏁 Script executed: # Look at the database test to see how the types are actually used
cat apps/api/src/server/database.test.tsRepository: rohan-patnaik/opencalendly Length of output: 1011 🏁 Script executed: # Check the base tsconfig for strict settings
cat tsconfig.base.json | grep -A20 '"compilerOptions"'Repository: rohan-patnaik/opencalendly Length of output: 472 🏁 Script executed: # Check the CI workflow to see if it runs typecheck
cat .github/workflows/ci.yml | head -80Repository: rohan-patnaik/opencalendly Length of output: 1722 🏁 Script executed: # Verify the actual Drizzle type definitions to understand if Client and Pool return compatible types
rg -n '\$client' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Let's try to understand the Drizzle return types better by checking imports
rg -n 'import.*drizzle' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 124 🏁 Script executed: # Check if there's a type mismatch by looking at what Drizzle exports
rg -n 'NodePgDatabase|type.*Database' packages/db/src/client.tsRepository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Try running the typecheck to see if it actually fails
cd apps/api && npm run typecheck 2>&1 | head -50Repository: rohan-patnaik/opencalendly Length of output: 813 🏁 Script executed: # Let's check if we can install dependencies first and then typecheck with full context
npm install 2>&1 | tail -20Repository: rohan-patnaik/opencalendly Length of output: 914 🏁 Script executed: # Check the actual Drizzle version to understand the type behavior
cat packages/db/package.json | grep -i drizzleRepository: rohan-patnaik/opencalendly Length of output: 260 🏁 Script executed: # Look for any type exports or internal types that might indicate Client vs Pool incompatibility
rg -n 'export type|interface.*Database' packages/db/srcRepository: rohan-patnaik/opencalendly Length of output: 52 🏁 Script executed: # Try typecheck again now that dependencies are installed
cd apps/api && npm run typecheck 2>&1Repository: rohan-patnaik/opencalendly Length of output: 1192 🏁 Script executed: # Let's examine the actual type structure more carefully to understand if this is truly incompatible
rg -n 'schema' packages/db/src/client.ts | head -10Repository: rohan-patnaik/opencalendly Length of output: 250 This localhost branch does not type-check against The code fails CI with TS2379 at line 74: Widen the shared 🧰 Tools🪛 GitHub Actions: CI[error] 74-74: TypeScript (TS2379) type mismatch in typecheck. Argument of type 'NodePgDatabase<...> & { $client: Client; }' is not assignable to parameter type 'NodePgDatabase<...> & { $client: Pool; }' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties. 🤖 Prompt for AI Agents |
||
|
|
||
| const { db } = createRuntimeDb(connection.connectionString, { | ||
| enforceNeon: connection.source === 'database_url', | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1532,3 +1532,23 @@ Acceptance criteria: | |
| - Validation passes: | ||
| - `git diff --check` | ||
| - workflow YAML remains syntactically valid | ||
|
|
||
| ### Feature 59: Local auth safety on fixed dev ports | ||
|
|
||
| Scope: | ||
| - Prevent local auth and OAuth flows from silently breaking when the web or API dev server starts on an unexpected fallback port. | ||
| - Keep local development pinned to `localhost:3000` for web and `localhost:8787` for API. | ||
| - Fail fast with a clear error when either expected port is already occupied. | ||
| - Keep the pooled local API database runtime compatible with wrangler/workerd timer behavior so auth session exchange does not fail under local development. | ||
|
|
||
| Acceptance criteria: | ||
|
|
||
| - `npm run dev:web` checks port `3000` first and exits with a clear message instead of silently switching to another port. | ||
| - `npm run dev:api` checks port `8787` first and exits with a clear message instead of silently switching to another port. | ||
| - Local auth/OAuth assumptions remain aligned with `.env` defaults for `APP_BASE_URL` and `NEXT_PUBLIC_API_BASE_URL`. | ||
| - Local API auth/session requests do not fail with `tid.unref is not a function` from pooled database cleanup. | ||
| - Local localhost auth/session requests remain stable under wrangler/workerd instead of hanging after `POST /v0/auth/clerk/exchange`. | ||
| - Validation passes: | ||
| - `npm run lint` | ||
| - `npm run test` | ||
| - `npm run typecheck` | ||
|
Comment on lines
+1536
to
+1554
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renumber this backlog entry.
🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { runtimeSupportsTimerUnref } from './client'; | ||
|
|
||
| describe('runtimeSupportsTimerUnref', () => { | ||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it('returns true when timers expose unref', () => { | ||
| const unref = vi.fn(); | ||
| const timer = { unref }; | ||
| const clearTimeoutMock = vi.fn(); | ||
|
|
||
| vi.stubGlobal('setTimeout', vi.fn(() => timer) as unknown as typeof setTimeout); | ||
| vi.stubGlobal('clearTimeout', clearTimeoutMock as unknown as typeof clearTimeout); | ||
|
|
||
| expect(runtimeSupportsTimerUnref()).toBe(true); | ||
| expect(clearTimeoutMock).toHaveBeenCalledWith(timer); | ||
| }); | ||
|
|
||
| it('returns false when timers do not expose unref', () => { | ||
| const clearTimeoutMock = vi.fn(); | ||
|
|
||
| vi.stubGlobal('setTimeout', vi.fn(() => 1) as unknown as typeof setTimeout); | ||
| vi.stubGlobal('clearTimeout', clearTimeoutMock as unknown as typeof clearTimeout); | ||
|
|
||
| expect(runtimeSupportsTimerUnref()).toBe(false); | ||
| expect(clearTimeoutMock).toHaveBeenCalledWith(1); | ||
| }); | ||
| }); | ||
|
Comment on lines
+5
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cover the This suite proves Based on learnings: Applies to **/*.{test,spec}.{js,ts,tsx} : Add/update tests with each implementation change. 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,26 @@ | ||||||||||||||||||||||||||||||||
| import net from 'node:net'; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const [rawPort, ...labelParts] = process.argv.slice(2); | ||||||||||||||||||||||||||||||||
| const port = Number.parseInt(rawPort ?? '', 10); | ||||||||||||||||||||||||||||||||
| const label = labelParts.join(' ').trim() || `port ${rawPort ?? 'unknown'}`; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!Number.isInteger(port) || port <= 0) { | ||||||||||||||||||||||||||||||||
| console.error('Usage: node scripts/ensure-dev-port.mjs <port> [label]'); | ||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
node --input-type=module <<'EOF'
for (const raw of ['3000abc', '65536']) {
const port = Number.parseInt(raw, 10);
console.log({
raw,
parsed: port,
passesCurrentGuard: Number.isInteger(port) && port > 0,
});
}
EOFRepository: rohan-patnaik/opencalendly Length of output: 186 🏁 Script executed: head -15 scripts/ensure-dev-port.mjsRepository: rohan-patnaik/opencalendly Length of output: 588 Validate the raw port token more strictly. Line 4 uses Suggested tightening-const [rawPort, ...labelParts] = process.argv.slice(2);
-const port = Number.parseInt(rawPort ?? '', 10);
+const [rawPort, ...labelParts] = process.argv.slice(2);
+const normalizedPort = rawPort?.trim() ?? '';
+const port = /^\d+$/.test(normalizedPort) ? Number(normalizedPort) : Number.NaN;
const label = labelParts.join(' ').trim() || `port ${rawPort ?? 'unknown'}`;
-if (!Number.isInteger(port) || port <= 0) {
+if (!Number.isInteger(port) || port <= 0 || port > 65_535) {
console.error('Usage: node scripts/ensure-dev-port.mjs <port> [label]');
process.exit(1);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const server = net.createServer(); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| server.once('error', (error) => { | ||||||||||||||||||||||||||||||||
| if ((error && typeof error === 'object' && 'code' in error && error.code) === 'EADDRINUSE') { | ||||||||||||||||||||||||||||||||
| console.error(`${label} is already in use. Stop the existing process before starting local dev.`); | ||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| console.error(`Unable to verify ${label}: ${error instanceof Error ? error.message : 'unknown error'}`); | ||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| server.listen(port, () => { | ||||||||||||||||||||||||||||||||
| server.close(() => process.exit(0)); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
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.
Add coverage for the new
withDatabaselocalhost branch.These tests only lock the hostname predicate. The PR's behavior change is the
createDb/connect/endpath inwithDatabase, and nothing here asserts that localhost requests actually take that branch and clean up the client. A small mock-based test would catch regressions that reintroduce the hanging local auth flow.Based on learnings: Tests added/updated as part of Definition of Done.
🤖 Prompt for AI Agents