-
Notifications
You must be signed in to change notification settings - Fork 22
feat(auth-session-mgmt): align with blog — connector OCC retry, async-job wait, housekeeping, conn lifetime #1287
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
Changes from 2 commits
7df8b6f
83d567c
3ed9457
5cb4502
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 |
|---|---|---|
|
|
@@ -28,13 +28,15 @@ let pool: AuroraDSQLPool | null = null; | |
| * Returns the shared DSQL connection pool, creating it on first access. | ||
| * | ||
| * The pool is configured with: | ||
| * - `host` — read from the `DSQL_ENDPOINT` environment variable | ||
| * - `user` — `'admin'` (DSQL's default IAM-authenticated user) | ||
| * - `database` — `'postgres'` (DSQL's fixed database name) | ||
| * - `max` — 10 concurrent connections | ||
| * - `idleTimeoutMillis` — 300 000 ms (5 minutes), well under the 1-hour | ||
| * DSQL connection timeout so connections are recycled | ||
| * before they expire | ||
| * - `host` — read from the `DSQL_ENDPOINT` environment variable | ||
| * - `user` — `'admin'` (DSQL's default IAM-authenticated user) | ||
| * - `database` — `'postgres'` (DSQL's fixed database name) | ||
| * - `max` — 10 concurrent connections | ||
| * - `idleTimeoutMillis` — 300 000 ms (5 minutes), well under the 1-hour | ||
| * DSQL connection timeout so connections are | ||
| * recycled before they expire | ||
| * - `maxLifetimeSeconds` — 3 300 s (55 minutes), so each connection retires | ||
| * ahead of DSQL's hard 1-hour cap | ||
| * | ||
| * @throws {Error} If `DSQL_ENDPOINT` is not set in the environment. | ||
| */ | ||
|
|
@@ -56,8 +58,26 @@ export function getPool(): AuroraDSQLPool { | |
| database: 'postgres', | ||
| max: 10, | ||
| idleTimeoutMillis: 300_000, // 5 minutes | ||
| // Matches the connector default in @aws/aurora-dsql-node-postgres-connector | ||
| // v0.1.9 (parsePgConfig sets maxLifetimeSeconds: 3300 unless overridden); | ||
| // kept here for visibility so readers see the 1-hour cap accommodation | ||
| // without having to grep the connector source. | ||
| maxLifetimeSeconds: 3300, | ||
|
Contributor
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. FYI the connector's |
||
| }); | ||
|
|
||
| // Production guarantee: AuroraDSQLPool always exposes transaction(), and | ||
| // the repositories rely on it for OCC retry. Surface a clear error here | ||
| // if a future refactor accidentally returns a plain pg.Pool, rather than | ||
| // silently degrading to the manual BEGIN/COMMIT fallback that exists for | ||
| // unit-test mocks. | ||
| if (typeof (pool as { transaction?: unknown }).transaction !== 'function') { | ||
| throw new Error( | ||
| 'Pool does not expose transaction(); expected AuroraDSQLPool from ' + | ||
| '@aws/aurora-dsql-node-postgres-connector. Repositories rely on ' + | ||
| 'pool.transaction() for OCC retry.', | ||
| ); | ||
| } | ||
|
|
||
| return pool; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,17 +7,25 @@ import { runMigrations, PoolLike, ClientLike } from './migrate'; | |
|
|
||
| /** | ||
| * Creates a mock client that records every SQL statement it receives. | ||
| * Optionally accepts a callback that can throw to simulate failures. | ||
| * | ||
| * Returns `{ rows: [{ job_id: 'fake-job-id' }] }` for `CREATE INDEX ASYNC` | ||
| * statements (matching the real DSQL behaviour that returns a job_id row), | ||
| * and `{ rows: [] }` for everything else. Tests that need a different shape | ||
| * can pass a custom `onQuery` to override. | ||
| */ | ||
| function createMockClient( | ||
| onQuery?: (sql: string) => void | ||
| onQuery?: (sql: string) => void, | ||
| ): ClientLike & { queries: string[] } { | ||
| const queries: string[] = []; | ||
| return { | ||
| queries, | ||
| query: vi.fn(async (sql: string) => { | ||
| query: vi.fn(async (sql: string, _params?: unknown[]) => { | ||
| queries.push(sql); | ||
| onQuery?.(sql); | ||
| if (sql.startsWith('CREATE INDEX ASYNC')) { | ||
| return { rows: [{ job_id: 'fake-job-id' }] }; | ||
| } | ||
| return { rows: [] }; | ||
| }), | ||
| release: vi.fn(), | ||
| }; | ||
|
|
@@ -60,17 +68,22 @@ describe('runMigrations', () => { | |
|
|
||
| await runMigrations(pool); | ||
|
|
||
| // We expect 3 separate transactions: users table, sessions table, | ||
| // and the user_id index. The token_hash UNIQUE constraint already | ||
| // creates a backing index, so we deliberately do NOT add a second. | ||
| expect(clients).toHaveLength(3); | ||
| // 3 DDL transactions (users table, sessions table, user_id index) plus | ||
| // a 4th client for the post-DDL `sys.wait_for_job` call after the | ||
| // async index. The token_hash UNIQUE constraint already creates a | ||
| // backing index, so we deliberately do NOT add a second. | ||
| expect(clients).toHaveLength(4); | ||
|
|
||
| // Each client should have received BEGIN → DDL → COMMIT | ||
| for (const client of clients) { | ||
| // First three clients ran BEGIN → DDL → COMMIT. | ||
| for (const client of clients.slice(0, 3)) { | ||
| expect(client.queries[0]).toBe('BEGIN'); | ||
| expect(client.queries[2]).toBe('COMMIT'); | ||
| expect(client.queries).toHaveLength(3); | ||
| } | ||
|
|
||
| // The 4th client ran the wait_for_job for the async index. | ||
| expect(clients[3].queries).toHaveLength(1); | ||
| expect(clients[3].queries[0]).toBe('SELECT sys.wait_for_job($1)'); | ||
|
Contributor
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. The mock client's expect(clients[3].query).toHaveBeenCalledWith(
'SELECT sys.wait_for_job($1)',
['fake-job-id'],
); |
||
| }); | ||
|
|
||
| it('creates the users table first', async () => { | ||
|
|
@@ -98,7 +111,7 @@ describe('runMigrations', () => { | |
| expect(ddl).toContain('token_hash VARCHAR(64) NOT NULL UNIQUE'); | ||
| expect(ddl).toContain('expires_at TIMESTAMPTZ NOT NULL'); | ||
| expect(ddl).toContain('revoked_at TIMESTAMPTZ'); | ||
| expect(ddl).toContain('client_metadata JSONB'); | ||
| expect(ddl).toContain('client_metadata TEXT'); | ||
| }); | ||
|
|
||
| it('creates the user_id index third', async () => { | ||
|
|
@@ -116,11 +129,34 @@ describe('runMigrations', () => { | |
|
|
||
| await runMigrations(pool); | ||
|
|
||
| const allDdl = clients.map((c) => c.queries[1]).join('\n'); | ||
| const allDdl = clients.slice(0, 3).map((c) => c.queries[1]).join('\n'); | ||
| expect(allDdl).not.toContain('idx_sessions_token_hash'); | ||
| expect(allDdl).not.toMatch(/CREATE INDEX[^\n]*ON sessions \(token_hash\)/); | ||
| }); | ||
|
|
||
| it('waits for the async index job to complete', async () => { | ||
| const { pool, clients } = createMockPool(); | ||
|
|
||
| await runMigrations(pool); | ||
|
|
||
| // The 4th client (after the 3 DDL clients) ran SELECT sys.wait_for_job($1). | ||
| // Assert on the params too so a regression that passes undefined for $1 | ||
| // would fail the test. | ||
| expect(clients[3].query).toHaveBeenCalledWith( | ||
| 'SELECT sys.wait_for_job($1)', | ||
| ['fake-job-id'], | ||
| ); | ||
| }); | ||
|
|
||
| it('skips wait_for_job when waitForAsyncJobs is false', async () => { | ||
| const { pool, clients } = createMockPool(); | ||
|
|
||
| await runMigrations(pool, { waitForAsyncJobs: false }); | ||
|
|
||
| // Only the 3 DDL clients — no wait_for_job client. | ||
| expect(clients).toHaveLength(3); | ||
| }); | ||
|
|
||
| it('releases every client back to the pool', async () => { | ||
| const { pool, clients } = createMockPool(); | ||
|
|
||
|
|
@@ -135,47 +171,48 @@ describe('runMigrations', () => { | |
| // Error handling | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| it('rolls back and re-throws when a DDL statement fails', async () => { | ||
| const ddlError = new Error('relation already exists'); | ||
| let callCount = 0; | ||
|
|
||
| const failingPool: PoolLike = { | ||
| it('rolls back the transaction when a DDL statement fails', async () => { | ||
| const clients: ReturnType<typeof createMockClient>[] = []; | ||
| const pool: PoolLike = { | ||
| connect: vi.fn(async () => { | ||
| callCount++; | ||
| // Fail on the second DDL (sessions table) | ||
| if (callCount === 2) { | ||
| return createMockClient((sql) => { | ||
| if (sql !== 'BEGIN' && sql !== 'ROLLBACK') { | ||
| throw ddlError; | ||
| } | ||
| }); | ||
| } | ||
| return createMockClient(); | ||
| const indexBeingCreated = clients.length; // 0,1,2,... | ||
| const client = createMockClient((sql) => { | ||
| // Throw on the second client's DDL (the sessions table CREATE) | ||
| if (indexBeingCreated === 1 && sql.startsWith('CREATE TABLE')) { | ||
| throw new Error('simulated DDL failure'); | ||
| } | ||
| }); | ||
| clients.push(client); | ||
| return client; | ||
| }), | ||
| }; | ||
|
|
||
| await expect(runMigrations(failingPool)).rejects.toThrow( | ||
| 'relation already exists' | ||
| ); | ||
| }); | ||
| await expect(runMigrations(pool)).rejects.toThrow('simulated DDL failure'); | ||
|
|
||
| it('rolls back the transaction on failure before releasing the client', async () => { | ||
| const ddlError = new Error('syntax error'); | ||
| const failingClient = createMockClient((sql) => { | ||
| if (sql !== 'BEGIN' && sql !== 'ROLLBACK') { | ||
| throw ddlError; | ||
| } | ||
| }); | ||
| // Second client should have run BEGIN, attempted DDL, and ROLLBACK. | ||
| const secondClient = clients[1]; | ||
| expect(secondClient.queries).toContain('BEGIN'); | ||
| expect(secondClient.queries).toContain('ROLLBACK'); | ||
| }); | ||
|
|
||
| const failingPool: PoolLike = { | ||
| connect: vi.fn(async () => failingClient), | ||
| it('still releases the client when the DDL fails', async () => { | ||
| const clients: ReturnType<typeof createMockClient>[] = []; | ||
| const pool: PoolLike = { | ||
| connect: vi.fn(async () => { | ||
| const indexBeingCreated = clients.length; | ||
| const client = createMockClient((sql) => { | ||
| // Fail the very first DDL (users table CREATE) | ||
| if (indexBeingCreated === 0 && sql.startsWith('CREATE TABLE')) { | ||
| throw new Error('simulated failure'); | ||
| } | ||
| }); | ||
| clients.push(client); | ||
| return client; | ||
| }), | ||
| }; | ||
|
|
||
| await expect(runMigrations(failingPool)).rejects.toThrow('syntax error'); | ||
| await expect(runMigrations(pool)).rejects.toThrow('simulated failure'); | ||
|
|
||
| // Should have attempted ROLLBACK after the failure | ||
| expect(failingClient.queries).toContain('ROLLBACK'); | ||
| // Client must still be released even after an error | ||
| expect(failingClient.release).toHaveBeenCalledTimes(1); | ||
| expect(clients[0].release).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
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.
Quick note for the undo path — verified live:
DROP ROLE app_runtimefails with2BP01 cannot be dropped because some objects depend on itif you don't firstAWS IAM REVOKE app_runtime FROM '<arn>'. Worth one line at the end of this bullet so readers can roll the role back cleanly.