External db sync #1036
External db sync #1036
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements an external database synchronization system that enables syncing internal database changes (ProjectUser, ContactChannel, DeletedRow) to external PostgreSQL databases via sequence-based change tracking, cron-based polling infrastructure, and Upstash webhook integration. Changes
Sequence DiagramsequenceDiagram
participant Cron as Cron Scheduler<br/>(run-cron-jobs.ts)
participant Seq as Sequencer Route
participant IDB as Internal DB<br/>(Prisma)
participant OReq as OutgoingRequest<br/>Table
participant Poll as Poller Route
participant Upstash as Upstash<br/>Queue
participant Sync as Sync Engine<br/>Route
participant EDB as External DB<br/>(PostgreSQL)
Cron->>Seq: GET /sequencer (cron secret)
Seq->>IDB: Backfill null sequenceIds<br/>in batches (ProjectUser,<br/>ContactChannel, DeletedRow)
Seq->>IDB: Fetch affected tenancyIds
Seq->>OReq: Enqueue OutgoingRequest<br/>per tenant (if not pending)
Seq->>Cron: 200 { ok, iterations }
Cron->>Poll: GET /poller (cron secret)
Poll->>OReq: Claim up to 100 pending<br/>requests (SKIP LOCKED,<br/>3-min window)
Poll->>Upstash: Publish per-request<br/>payload to queue
Poll->>Cron: 200 { ok, total }
Upstash->>Sync: POST /sync-engine<br/>(Upstash signature,<br/>tenancyId in body)
Sync->>IDB: Fetch tenancy config
Sync->>IDB: Call syncExternalDatabases
IDB->>EDB: Fetch internal rows<br/>since lastSequenceId
IDB->>EDB: Upsert rows + metadata
Sync->>OReq: Mark OutgoingRequest<br/>as fulfilled
Sync->>Upstash: 200 { ok, statusCode }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This reverts commit 937db90.
There was a problem hiding this comment.
can you merge all these migrations into a single one and document what each of the steps do?
There was a problem hiding this comment.
done - merged them into one and added comments
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive external database synchronization system to replicate data from Stack's internal database to user-configured external databases in near real-time.
Key changes:
- Adds sequence ID tracking for ProjectUser, ContactChannel, and DeletedRow tables
- Implements a sequencer that assigns sequence IDs to track change order
- Implements a poller that queues sync requests via QStash
- Implements a sync engine that pulls changes and pushes them to external databases
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vercel.json | Adds cron jobs for sequencer and poller endpoints (runs every minute) |
| pnpm-lock.yaml | Updates package dependencies including pg client library |
| package.json | Adds @types/pg dependency for TypeScript support |
| apps/e2e/package.json | Adds pg and @types/pg for E2E testing |
| docker/dependencies/docker.compose.yaml | Adds external-db-test PostgreSQL service for testing |
| apps/backend/prisma/schema.prisma | Adds sequenceId fields, OutgoingRequest and DeletedRow tables |
| apps/backend/prisma/migrations/* | Database migrations for sequence IDs, triggers, and sync tables |
| packages/stack-shared/src/config/schema.ts | Adds dbSync configuration schema for external databases |
| packages/stack-shared/src/config/db-sync-mappings.ts | Defines default SQL mappings for PartialUsers table |
| apps/backend/src/lib/external-db-sync.ts | Core sync logic for pushing data to external databases |
| apps/backend/src/app/api/latest/internal/external-db-sync/* | API endpoints for sequencer, poller, and sync engine |
| apps/backend/scripts/run-cron-jobs.ts | Local development script to simulate cron jobs |
| apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-*.test.ts | Comprehensive E2E tests for basic, advanced, and race condition scenarios |
| apps/backend/src/route-handlers/smart-route-handler.tsx | Removes extensive logging from API requests |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error( | ||
| ` Column count mismatch for table "${tableName}".\n` + | ||
| `→ upsertQuery expects ${expectedParamCount} parameters.\n` + | ||
| `→ internalDbFetchQuery returned ${orderedKeys.length} columns (excluding tenancyId).\n` + | ||
| `Fix your SELECT column order or your SQL parameter order.` | ||
| ); |
There was a problem hiding this comment.
The error message has inconsistent formatting with a leading space on line 41 but not on lines 42-44. Consider removing the leading space for consistency.
| const rawSourceTables: any = (mapping as any).sourceTables; | ||
| const sourceTables: string[] = rawSourceTables | ||
| ? Object.values(rawSourceTables) | ||
| : []; | ||
|
|
||
| const rawTargetPk: any = (mapping as any).targetTablePrimaryKey; |
There was a problem hiding this comment.
The type casting with as any bypasses type safety. Consider defining proper TypeScript interfaces for sourceTables and targetTablePrimaryKey to maintain type safety throughout the codebase.
| const rawSourceTables: any = (mapping as any).sourceTables; | |
| const sourceTables: string[] = rawSourceTables | |
| ? Object.values(rawSourceTables) | |
| : []; | |
| const rawTargetPk: any = (mapping as any).targetTablePrimaryKey; | |
| const rawSourceTables = mapping.sourceTables; | |
| const sourceTables: string[] = rawSourceTables | |
| ? Object.values(rawSourceTables) | |
| : []; | |
| const rawTargetPk = mapping.targetTablePrimaryKey; |
| const rows = await tx.$queryRaw<OutgoingRequest[]>` | ||
| UPDATE "OutgoingRequest" | ||
| SET "fulfilledAt" = NOW() | ||
| WHERE "id" IN ( | ||
| SELECT id | ||
| FROM "OutgoingRequest" | ||
| WHERE "fulfilledAt" IS NULL | ||
| ORDER BY "createdAt" | ||
| LIMIT 100 | ||
| FOR UPDATE SKIP LOCKED | ||
| ) | ||
| RETURNING *; | ||
| `; | ||
| return rows; | ||
| }); |
There was a problem hiding this comment.
The FOR UPDATE SKIP LOCKED clause prevents multiple pollers from processing the same requests, but there's no guarantee that all pending requests will be processed if the poller times out. Consider adding a mechanism to track and retry requests that were claimed but never completed (e.g., where fulfilledAt is set but the QStash publish failed).
| 'Poller does not see second update until commit', | ||
| async () => { | ||
| const dbName = 'race_second_uncommitted_poll_test'; | ||
| const { externalClient, user, baselineSeq } = |
There was a problem hiding this comment.
Unused variable baselineSeq.
| const { externalClient, user, baselineSeq } = | |
| const { externalClient, user } = |
| @@ -0,0 +1,1108 @@ | |||
| import { DEFAULT_DB_SYNC_MAPPINGS } from '@stackframe/stack-shared/dist/config/db-sync-mappings'; | |||
| import { generateSecureRandomString } from '@stackframe/stack-shared/dist/utils/crypto'; | |||
There was a problem hiding this comment.
Unused import generateSecureRandomString.
| import { generateSecureRandomString } from '@stackframe/stack-shared/dist/utils/crypto'; |
| import { DEFAULT_EMAIL_TEMPLATES, DEFAULT_EMAIL_THEMES, DEFAULT_EMAIL_THEME_ID } from "../helpers/emails"; | ||
| import * as schemaFields from "../schema-fields"; | ||
| import { productSchema, userSpecifiedIdSchema, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields"; | ||
| import { productSchema, userSpecifiedIdSchema, yupArray, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields"; |
There was a problem hiding this comment.
Unused import yupArray.
| import { productSchema, userSpecifiedIdSchema, yupArray, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields"; | |
| import { productSchema, userSpecifiedIdSchema, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields"; |
Greptile OverviewGreptile SummaryThis PR implements a comprehensive external database synchronization system that continuously replicates changes from Stack's internal database to user-configured external databases. The implementation uses a sequence-based approach with four main components: a sequencer that assigns monotonically increasing IDs to changed rows, a poller that queues sync requests, a QStash-based queue for reliable delivery, and a sync engine that executes the actual data transfer using user-defined SQL mappings. Key Changes:
Architecture Strengths:
Concerns:
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User as User Action
participant Internal as Internal DB
participant Trigger as DB Triggers
participant Sequencer as Sequencer<br/>(Cron: Every 1min)
participant Poller as Poller<br/>(Cron: Every 1min)
participant Queue as OutgoingRequest<br/>Table
participant QStash as QStash Queue
participant SyncEngine as Sync Engine
participant External as External DB
User->>Internal: INSERT/UPDATE/DELETE User/Contact
Internal->>Trigger: Fire trigger
Trigger->>Internal: Set shouldUpdateSequenceId=TRUE
Note over Sequencer: Runs every 50ms for 2min
Sequencer->>Internal: Call backfill_null_sequence_ids()
Internal->>Internal: Assign sequenceId to changed rows
Internal->>Internal: Set shouldUpdateSequenceId=FALSE
Internal->>Queue: INSERT into OutgoingRequest<br/>(via enqueue_tenant_sync)
Note over Poller: Runs every 50ms for 2min
Poller->>Queue: SELECT pending requests<br/>(FOR UPDATE SKIP LOCKED)
Queue->>Poller: Return up to 100 requests
Poller->>Queue: UPDATE fulfilledAt=NOW()
Poller->>QStash: publishJSON(sync-engine endpoint)
QStash->>SyncEngine: POST /sync-engine
SyncEngine->>SyncEngine: Verify Upstash signature
SyncEngine->>Internal: Execute internalDbFetchQuery<br/>(with tenancyId parameter)
Internal->>SyncEngine: Return changed rows (ordered by sequenceId)
SyncEngine->>External: Connect to external DB
SyncEngine->>External: Create table if needed<br/>(targetTableSchema)
loop For each row
SyncEngine->>External: Execute externalDbUpdateQuery<br/>(upsert with sequence check)
end
External->>External: Update only if newSeq > oldSeq
SyncEngine->>QStash: Return success
|
| } | ||
| } | ||
|
|
||
| const rows = await internalPrisma.$queryRawUnsafe<any[]>(fetchQuery, tenancyId); |
There was a problem hiding this comment.
logic: SQL injection vulnerability: fetchQuery comes from user-controlled config (mapping.internalDbFetchQuery) and is executed with $queryRawUnsafe. Malicious SQL in config could read/modify internal database data.
| const rows = await internalPrisma.$queryRawUnsafe<any[]>(fetchQuery, tenancyId); | |
| const rows = await internalPrisma.$queryRaw<any[]>`${fetchQuery}` as any; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/lib/external-db-sync.ts
Line: 133:133
Comment:
**logic:** SQL injection vulnerability: `fetchQuery` comes from user-controlled config (`mapping.internalDbFetchQuery`) and is executed with `$queryRawUnsafe`. Malicious SQL in config could read/modify internal database data.
```suggestion
const rows = await internalPrisma.$queryRaw<any[]>`${fetchQuery}` as any;
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (20)
apps/backend/prisma/migrations/20251125030551_add_sequence_id/migration.sql (1)
1-6: Consider documenting the sequence increment strategy.The
INCREMENT BY 11in the global sequence is unusual and likely implements a per-instance ID distribution strategy for multi-instance deployments. Add an inline SQL comment explaining this choice for future maintainers.Ref: Past review feedback suggested consolidating all sequence-related migrations and documenting each step. Consider grouping this with the subsequent
shouldUpdateSequenceIdand index migrations into a single, well-documented migration file.-- Add this comment to explain the strategy CREATE SEQUENCE global_seq_id AS BIGINT START 1 INCREMENT BY 11 -- Increment by 11 to enable per-instance ID ranges (e.g., instance 0: 1,12,23...; instance 1: 2,13,24...) in multi-instance deployments NO MINVALUE NO MAXVALUE;apps/backend/prisma/migrations/20251128210001_add_should_update_sequence_id_indexes/migration.sql (1)
1-7: LGTM!The partial indexes on
shouldUpdateSequenceId = TRUEare an efficient choice for queries filtering unsynced records, reducing index size by excluding already-synced rows. The naming and SPLIT_STATEMENT_SENTINEL structure are consistent with related migrations.Consider adding an inline SQL comment explaining the partial index strategy:
-- Partial index for efficient querying of unsynced records CREATE INDEX "ProjectUser_shouldUpdateSequenceId_idx" ON "ProjectUser"("shouldUpdateSequenceId") WHERE "shouldUpdateSequenceId" = TRUE;apps/backend/prisma/migrations/20251128210000_add_should_update_sequence_id_columns/migration.sql (1)
1-7: LGTM!The addition of
shouldUpdateSequenceIdwith defaultTRUEacross ProjectUser, ContactChannel, and DeletedRow ensures all existing records are marked for synchronization to external databases. This flag enables efficient querying of unsynced records paired with the partial indexes in the follow-up migration.Add inline SQL comments to clarify the column's purpose:
-- Track which records need to be synced to external databases (reset to FALSE after sync) ALTER TABLE "ProjectUser" ADD COLUMN "shouldUpdateSequenceId" BOOLEAN NOT NULL DEFAULT TRUE;apps/backend/prisma/migrations/20251125180000_add_deleted_row_table/migration.sql (1)
8-9: Consider data retention strategy for fulfilled deletions.The
fulfilledAttimestamp tracks when deletions have been synced, but there's no documented retention policy or cleanup process for old rows. Over time, theDeletedRowtable could accumulate millions of fulfilled records, impacting query performance and storage.Recommend documenting a data retention policy (e.g., "delete rows where fulfilledAt < NOW() - INTERVAL '30 days'") and implementing a cleanup job.
apps/backend/prisma/migrations/20251125172224_add_outgoing_request/migration.sql (1)
2-12: Consider adding an index oncreatedAtfor query performance.The poller route orders by
createdAtwhen claiming pending requests (ORDER BY "createdAt"). As the table grows, this ordering operation on unfulfilled requests could benefit from a composite index.CREATE INDEX "OutgoingRequest_fulfilledAt_idx" ON "OutgoingRequest"("fulfilledAt"); + +CREATE INDEX "OutgoingRequest_createdAt_idx" ON "OutgoingRequest"("createdAt");Alternatively, a composite index
(fulfilledAt, createdAt)could further optimize the query pattern used in the poller.apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts (2)
81-87: Fragile environment detection pattern.Using
.includes("development")on the environment string is unusual and could match unintended values. The standard approach is direct equality comparison.-if (getNodeEnvironment().includes("development") || getNodeEnvironment().includes("test")) { +const env = getNodeEnvironment(); +if (env === "development" || env === "test") {
74-74: Consider defining a type forqstashOptions.Casting to
anyloses type safety. Define an interface for the expected shape ofqstashOptionsto catch errors at compile time.interface QstashOptions { url: string; body: unknown; } // ... const options = request.qstashOptions as QstashOptions;packages/stack-shared/src/config/schema.ts (1)
11-11: Unused import:yupArrayThe import
yupArraywas added but doesn't appear to be used in this file. Based on the code, onlyyupTupleis used for array-like structures (Line 216).-import { productSchema, userSpecifiedIdSchema, yupArray, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields"; +import { productSchema, userSpecifiedIdSchema, yupBoolean, yupDate, yupMixed, yupNever, yupNumber, yupObject, yupRecord, yupString, yupTuple, yupUnion } from "../schema-fields";apps/backend/scripts/run-cron-jobs.ts (2)
25-29: Cron jobs don't run immediately on startup.The
setIntervalschedules jobs to run after the first 60-second delay. Consider whether the jobs should also run immediately when the script starts:for (const endpoint of endpoints) { + run(endpoint); // Run immediately on startup setInterval(() => { run(endpoint); }, 60000); }
16-23: Consider adding a timeout to the fetch request.The
fetchcall has no timeout. If the backend hangs, this could cause the cron job to stall indefinitely. Per coding guidelines, blocking calls without timeouts can cause cascading issues.const run = (endpoint: string) => runAsynchronously(async () => { console.log(`Running ${endpoint}...`); - const res = await fetch(`${baseUrl}${endpoint}`, { - headers: { 'Authorization': `Bearer ${cronSecret}` }, - }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 120000); // 2 minute timeout + try { + const res = await fetch(`${baseUrl}${endpoint}`, { + headers: { 'Authorization': `Bearer ${cronSecret}` }, + signal: controller.signal, + }); + if (!res.ok) throw new StackAssertionError(`Failed to call ${endpoint}: ${res.status} ${res.statusText}\n${await res.text()}`, { res }); + console.log(`${endpoint} completed.`); + } finally { + clearTimeout(timeoutId); + } - if (!res.ok) throw new StackAssertionError(`Failed to call ${endpoint}: ${res.status} ${res.statusText}\n${await res.text()}`, { res }); - console.log(`${endpoint} completed.`); });apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts (1)
50-65: Consider adding a circuit breaker for repeated failures.The loop catches errors and continues (Lines 53-55), which could lead to repeatedly hitting the database with failing queries for the full 2-minute duration. Consider tracking consecutive failures and breaking early:
let iterations = 0; + let consecutiveFailures = 0; + const maxConsecutiveFailures = 10; while (performance.now() - startTime < maxDurationMs) { try { await globalPrismaClient.$executeRaw`SELECT backfill_null_sequence_ids()`; + consecutiveFailures = 0; // Reset on success } catch (error) { console.warn('[sequencer] Failed to run backfill_null_sequence_ids:', error); + consecutiveFailures++; + if (consecutiveFailures >= maxConsecutiveFailures) { + console.error('[sequencer] Too many consecutive failures, breaking loop'); + break; + } } iterations++; // ... }apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (1)
298-349: Consider parameterizing the long sleep duration.The 70-second sleep makes this test slow for CI. If the poller interval is configurable in tests, you could reduce the wait time proportionally. Alternatively, mark this as a "slow test" that runs only in nightly builds.
apps/backend/prisma/schema.prisma (1)
895-911: Consider adding an index onsequenceIdforDeletedRowif sync performance becomes a concern.The model has a partial index on
shouldUpdateSequenceId(migration 20251128210001) that helps the backfill function. The sync engine's query filters bytenancyIdandtableName, then orders bysequenceId. While the@uniqueconstraint onsequenceIdprovides an index, a composite index on(tenancyId, tableName, sequenceId)could optimize the sync fetch query ifDeletedRowvolumes grow significantly. This is an optional optimization rather than a requirement.apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts (3)
2-2: Unused import:generateSecureRandomStringThis import is not used anywhere in the file.
-import { generateSecureRandomString } from '@stackframe/stack-shared/dist/utils/crypto';
390-390: Missing test timeoutThis test lacks an explicit timeout, unlike other tests in this file. Consider adding
TEST_TIMEOUTfor consistency and to prevent hanging tests.- }); + }, TEST_TIMEOUT);
1011-1011: Preferslice()over deprecatedsubstr()
substr()is deprecated in favor ofslice()orsubstring().- const testRunId = `${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + const testRunId = `${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;apps/backend/src/lib/external-db-sync.ts (3)
48-69: Row-by-row processing may cause performance bottleneckEach row is processed with a separate
await externalClient.query()call. For high-volume syncs (e.g., the 200-user test), this results in 200+ sequential round-trips to the external database.Consider batching multiple rows into a single transaction or using multi-value inserts for better throughput.
+ // Batch rows within a transaction for better performance + await externalClient.query('BEGIN'); + try { for (const row of newRows) { const { tenancyId, ...rest } = row; await externalClient.query(upsertQuery, Object.values(rest)); } + await externalClient.query('COMMIT'); + } catch (err) { + await externalClient.query('ROLLBACK'); + throw err; + }
82-90: Type safety: Excessiveas anycastsUsing
as anyto accesssourceTablesandtargetTablePrimaryKeybypasses TypeScript's type checking. If theCompleteConfigtype is correct, consider updating it to include these properties properly, or use type guards.
192-203: Sequential database syncing limits throughputEach external database is synced sequentially. For tenants with multiple external DBs configured, this could be parallelized with
Promise.all()to reduce total sync time.- for (const [dbId, dbConfig] of Object.entries(externalDatabases)) { - await syncDatabase(dbId, dbConfig, internalPrisma, tenancy.id); - } + await Promise.all( + Object.entries(externalDatabases).map(([dbId, dbConfig]) => + syncDatabase(dbId, dbConfig, internalPrisma, tenancy.id) + ) + );apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (1)
209-215: Consider adding type forexternalDatabasesparameterThe
externalDatabasesparameter is typed asany. Consider using the proper type from the config schema for better type safety.-export async function createProjectWithExternalDb(externalDatabases: any, projectOptions?: { display_name?: string, description?: string }) { +export async function createProjectWithExternalDb( + externalDatabases: Record<string, { type: string; connectionString: string; mappings?: unknown }>, + projectOptions?: { display_name?: string; description?: string } +) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
apps/backend/package.json(2 hunks)apps/backend/prisma/migrations/20251125030551_add_sequence_id/migration.sql(1 hunks)apps/backend/prisma/migrations/20251125172224_add_outgoing_request/migration.sql(1 hunks)apps/backend/prisma/migrations/20251125180000_add_deleted_row_table/migration.sql(1 hunks)apps/backend/prisma/migrations/20251128210000_add_should_update_sequence_id_columns/migration.sql(1 hunks)apps/backend/prisma/migrations/20251128210001_add_should_update_sequence_id_indexes/migration.sql(1 hunks)apps/backend/prisma/migrations/20251128210002_add_reset_sequence_id_function_and_triggers/migration.sql(1 hunks)apps/backend/prisma/migrations/20251128210003_log_deleted_row_function_and_triggers/migration.sql(1 hunks)apps/backend/prisma/migrations/20251128210004_add_sync_functions/migration.sql(1 hunks)apps/backend/prisma/schema.prisma(4 hunks)apps/backend/scripts/run-cron-jobs.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx(1 hunks)apps/backend/src/lib/external-db-sync.ts(1 hunks)apps/backend/src/route-handlers/smart-route-handler.tsx(1 hunks)apps/e2e/package.json(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts(1 hunks)docker/dependencies/docker.compose.yaml(2 hunks)package.json(1 hunks)packages/stack-shared/src/config/db-sync-mappings.ts(1 hunks)packages/stack-shared/src/config/schema-fuzzer.test.ts(1 hunks)packages/stack-shared/src/config/schema.ts(4 hunks)vercel.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: For blocking alerts and errors, never usetoast, as they are easily missed by the user. Instead, use alerts
Keep hover/click transitions snappy and fast. Don't delay the action with a pre-transition; apply transitions after the action instead
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). UserunAsynchronouslyorrunAsynchronouslyWithAlertinstead for asynchronous error handling
When creating hover transitions, avoid hover-enter transitions and just use hover-exit transitions (e.g.,transition-colors hover:transition-none)
Use ES6 maps instead of records wherever possible
Files:
packages/stack-shared/src/config/schema-fuzzer.test.tsapps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tspackages/stack-shared/src/config/db-sync-mappings.tsapps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsxapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.tsapps/backend/src/route-handlers/smart-route-handler.tsxapps/backend/scripts/run-cron-jobs.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/lib/external-db-sync.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.tspackages/stack-shared/src/config/schema.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When writing tests, prefer .toMatchInlineSnapshot over other selectors, if possible
Files:
packages/stack-shared/src/config/schema-fuzzer.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts
apps/backend/src/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The project uses a custom route handler system in the backend for consistent API responses
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsxapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
apps/{backend,dashboard}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if you can avoid them. Instead, prefer using a client component and prefer
usePathnameinstead ofawait params
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsxapps/backend/src/route-handlers/smart-route-handler.tsxapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/lib/external-db-sync.ts
packages/stack-shared/src/config/schema.ts
📄 CodeRabbit inference engine (AGENTS.md)
Whenever making backwards-incompatible changes to the config schema, update the migration functions in
packages/stack-shared/src/config/schema.ts
Files:
packages/stack-shared/src/config/schema.ts
🧠 Learnings (4)
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`
Applied to files:
packages/stack-shared/src/config/schema-fuzzer.test.tspackages/stack-shared/src/config/db-sync-mappings.tsapps/backend/prisma/migrations/20251128210004_add_sync_functions/migration.sqlapps/backend/package.jsonapps/backend/prisma/schema.prismaapps/backend/src/lib/external-db-sync.tspackages/stack-shared/src/config/schema.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to apps/backend/src/app/api/**/*.{ts,tsx} : The project uses a custom route handler system in the backend for consistent API responses
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsxapps/backend/src/route-handlers/smart-route-handler.tsxapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Always add new E2E tests when changing the API or SDK interface
Applied to files:
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to apps-{frontend,config}.ts{,x} : To update the list of apps available, edit `apps-frontend.tsx` and `apps-config.ts`
Applied to files:
packages/stack-shared/src/config/schema.ts
🧬 Code graph analysis (6)
apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx (5)
apps/backend/src/route-handlers/smart-route-handler.tsx (1)
createSmartRouteHandler(185-270)packages/stack-shared/src/schema-fields.ts (5)
yupObject(247-251)yupTuple(217-221)yupString(187-190)yupNumber(191-194)yupBoolean(195-198)apps/backend/src/lib/upstash.tsx (1)
ensureUpstashSignature(16-35)apps/backend/src/lib/tenancies.tsx (1)
getTenancy(82-91)apps/backend/src/lib/external-db-sync.ts (1)
syncExternalDatabases(192-203)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (2)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (6)
TestDbManager(16-67)createProjectWithExternalDb(209-215)waitForTable(150-169)waitForCondition(73-88)TEST_TIMEOUT(10-10)waitForSyncedDeletion(125-145)apps/e2e/tests/backend/backend-helpers.ts (1)
niceBackendFetch(109-173)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts (2)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (9)
TestDbManager(16-67)createProjectWithExternalDb(209-215)waitForSyncedData(93-120)verifyInExternalDb(182-189)TEST_TIMEOUT(10-10)waitForSyncedDeletion(125-145)verifyNotInExternalDb(174-177)waitForTable(150-169)waitForCondition(73-88)apps/e2e/tests/backend/backend-helpers.ts (1)
niceBackendFetch(109-173)
apps/backend/scripts/run-cron-jobs.ts (3)
packages/stack-shared/src/utils/env.tsx (1)
getEnvVariable(16-58)packages/stack-shared/src/utils/promises.tsx (1)
runAsynchronously(343-366)packages/stack-shared/src/utils/errors.tsx (1)
StackAssertionError(69-85)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts (2)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (9)
TestDbManager(16-67)createProjectWithExternalDb(209-215)waitForCondition(73-88)TEST_TIMEOUT(10-10)waitForTable(150-169)waitForSyncedData(93-120)HIGH_VOLUME_TIMEOUT(11-11)waitForSyncedDeletion(125-145)verifyNotInExternalDb(174-177)packages/stack-shared/src/config/db-sync-mappings.ts (1)
DEFAULT_DB_SYNC_MAPPINGS(1-166)
packages/stack-shared/src/config/schema.ts (2)
packages/stack-shared/src/schema-fields.ts (5)
yupObject(247-251)yupRecord(283-322)userSpecifiedIdSchema(442-442)yupString(187-190)yupTuple(217-221)packages/stack-shared/src/config/db-sync-mappings.ts (1)
DEFAULT_DB_SYNC_MAPPINGS(1-166)
🪛 GitHub Check: restart-dev-and-test
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
🪛 GitHub Check: restart-dev-and-test-with-custom-base-port
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
🪛 GitHub Check: setup-tests
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
[failure] 223-223:
Expected indentation of 2 spaces but found 4
[failure] 222-222:
Expected indentation of 4 spaces but found 6
[failure] 221-221:
Expected indentation of 2 spaces but found 4
[warning] 31-31:
Trailing spaces not allowed
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Vercel Agent Review
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (38)
package.json (1)
68-68: LGTM!The addition of
@types/pgto the monorepo root devDependencies provides TypeScript type support for PostgreSQL interactions across backend migrations, utilities, and end-to-end tests introduced in this PR.apps/e2e/package.json (1)
21-23: LGTM!The additions of
pgand@types/pgto e2e devDependencies enable end-to-end tests to interact with external PostgreSQL databases provisioned by the new external-db-test service. Version pinning is consistent with the monorepo root.docker/dependencies/docker.compose.yaml (1)
207-231: LGTM!The new
external-db-testPostgreSQL service follows the established patterns used by other test databases in the compose file (port mapping withNEXT_PUBLIC_STACK_PORT_PREFIX, volume persistence, hardcoded dev credentials). This isolates external DB sync tests from the main database and enables reproducible e2e testing.apps/backend/package.json (2)
10-10: LGTM!The addition of
run-cron-jobsto the dev concurrency labels integrates the background cron scheduler into local development, enabling testing of the sequencer and poller workflows that drive external DB synchronization.Please verify that
scripts/run-cron-jobs.tsproperly handles process signals (SIGTERM, SIGINT) so thatpnpm run devterminates cleanly without leaving orphaned cron processes.
38-39: LGTM!The
run-cron-jobsscript follows the established pattern: wrapped withwith-envfor environment setup and usestsxfor TypeScript execution. Dependencies (pgandtsx) are already present in the file.apps/backend/prisma/migrations/20251125180000_add_deleted_row_table/migration.sql (1)
15-19: LGTM!The indexes on
tenancyId,tableName, andsequenceIdare well-chosen for the expected query patterns: filtering by tenant, scanning deleted rows from specific tables, and correlating with sequence IDs for sync ordering.vercel.json (1)
2-11: LGTM!The cron configuration correctly schedules both the poller and sequencer endpoints to run every minute. This aligns with the internal route implementations that handle authentication via
CRON_SECRET.packages/stack-shared/src/config/schema-fuzzer.test.ts (1)
52-72: LGTM!The
dbSyncfuzzer configuration properly mirrors the new schema structure with representative test values for all fields includingexternalDatabases,mappings, and their nested properties.apps/backend/src/route-handlers/smart-route-handler.tsx (1)
101-102: Verify the logging removal is intentional.Based on the AI summary, this change removes development-time logging including request/response details and timing measurements. While this reduces log noise, ensure you have alternative observability (e.g., via Sentry spans at lines 71-80 and 232-248) to debug production issues.
apps/backend/prisma/migrations/20251128210003_log_deleted_row_function_and_triggers/migration.sql (2)
44-54: Triggers are correctly attached.Both triggers are appropriately configured as
BEFORE DELETE FOR EACH ROW, which ensures theOLDrecord is available for logging before the actual deletion occurs.
2-42: Well-structured trigger function for audit logging.The implementation correctly uses
BEFORE DELETEto captureOLDrow data and dynamically extracts primary key columns viapg_index/pg_attribute. Both ProjectUser and ContactChannel tables have the requiredtenancyIdcolumn, so the function will work as implemented. However, a couple of observations remain:
- If additional tables are added to this trigger function in the future without a
tenancyIdcolumn, the function will fail at runtime. Consider documenting this requirement or adding a check.- The dynamic PK extraction loop runs on every delete. For high-volume deletes, consider whether this overhead is acceptable or if caching the PK structure would be beneficial.
packages/stack-shared/src/config/schema.ts (2)
204-223: NewdbSyncschema added to branch config.The schema structure looks reasonable. A few observations:
connectionStringis marked asdefined()(required), but the defaults (Lines 594-595) settypeandconnectionStringtoundefined. This could cause validation issues when a database entry exists but isn't fully configured.The
targetTablePrimaryKeyusesyupTuple([yupString().defined()])which only allows exactly one string element. If composite primary keys with multiple columns are needed, consider usingyupArray(yupString().defined())instead.Per the coding guidelines for this file, backwards-incompatible changes to the config schema require migration functions in
migrateConfigOverride. Since this is a new field with defaults, existing configs should remain valid, but please confirm this is intentional.Based on learnings, whenever making backwards-incompatible changes to the config schema, update the migration functions. Please verify that adding
dbSyncdoesn't require a migration for existing projects.
591-598: Defaults fordbSync.externalDatabaseslook consistent with the schema.The default factory function returns
undefinedfortypeandconnectionString, withDEFAULT_DB_SYNC_MAPPINGSfor mappings. This aligns with the schema where these fields are optional until explicitly configured.apps/backend/scripts/run-cron-jobs.ts (1)
32-36: Error handling for main() follows project conventions.The
runAsynchronouslyusage for therunfunction and the.catch()onmain()withprocess.exit(1)is appropriate for a standalone script. The eslint disable comment acknowledges the intentional deviation.apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx (1)
7-31: Route handler structure follows project conventions.The use of
createSmartRouteHandlerwith metadata, request/response schemas aligns with the project's custom route handler system. The Upstash signature validation viaensureUpstashSignatureis appropriate for webhook security.apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts (2)
38-42: Auth check is straightforward and correct.The Bearer token comparison against
CRON_SECRETis appropriate for internal cron-triggered endpoints. The 401 StatusError on mismatch is the correct response.
14-37: Route handler metadata and schema are well-defined.The
hidden: trueflag appropriately hides this internal endpoint from public documentation. The response schema correctly reflects the returned structure.apps/backend/prisma/migrations/20251128210002_add_reset_sequence_id_function_and_triggers/migration.sql (1)
1-29: LGTM!The trigger logic correctly re-flags rows for sequence ID updates: when a processed row (
shouldUpdateSequenceId = FALSE) gets modified, the trigger resets it toTRUEso the sequencer picks it up again. TheBEFORE UPDATEtiming ensures the flag is set before the row is committed.apps/backend/prisma/migrations/20251128210004_add_sync_functions/migration.sql (1)
24-99: LGTM! Batched processing with concurrency-safe locking.The
FOR UPDATE SKIP LOCKEDpattern correctly handles concurrent sequencer invocations without deadlocks. Each table is limited to 1000 rows per invocation—ensure the sequencer cron runs frequently enough (50ms as documented) to drain backlogs during high-write periods.apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (1)
222-750: Well-structured race condition test suite.The tests comprehensively cover critical scenarios: uncommitted visibility, commit ordering, multi-row transactions, and sequence ID precedence. The
finallyblocks ensure proper client cleanup even on test failures.packages/stack-shared/src/config/db-sync-mappings.ts (2)
77-164: Correct sequence-based conflict resolution.The asymmetric comparison (
>=for deletes,>for upserts) ensures delete events at the same sequence ID take precedence—preventing stale data resurrection. TheON CONFLICTclause's final guard (EXCLUDED."sequenceId" > "PartialUsers"."sequenceId") adds a second layer of protection against out-of-order processing.
1-76: LGTM!The mapping correctly handles the
BooleanTrueenum conversion (line 34) and usesGREATEST()to track the maximum sequence ID across the joined tables. TheLIMIT 1000 ORDER BY sequenceIdenables paginated incremental fetching.apps/backend/prisma/schema.prisma (1)
176-178: LGTM!The schema additions are well-designed:
@uniqueonsequenceIdprevents duplicates,@default(true)onshouldUpdateSequenceIdensures new rows are automatically queued for sequencing, and theOutgoingRequestindex onfulfilledAtsupports efficient pending-request lookups.Also applies to: 258-260, 884-893
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts (6)
19-29: LGTM!Test setup and teardown are properly structured with
describe.sequentialto avoid concurrency issues, and theTestDbManagerlifecycle is correctly managed.
39-157: LGTM!Multi-tenant isolation test is thorough - it properly validates that User A only appears in Project A's databases and User B only in Project B's, with explicit negative assertions to confirm no cross-tenant leakage.
167-237: LGTM!The sequenceId tracking test correctly validates ordering guarantees using
BigIntfor comparison, which is appropriate for potentially large sequence values.
434-505: LGTM!The high-volume batching test is well-designed with retry logic, unique email generation, and appropriate timeout handling for the 200-user stress test.
625-692: LGTM!The write protection test properly validates that readonly PostgreSQL credentials prevent unauthorized modifications while still allowing reads - this is a valuable security verification.
883-980: LGTM!The complex lifecycle test thoroughly exercises the full create → update → update → delete → recreate → update sequence with proper sequenceId tracking and ID verification at each step.
apps/backend/src/lib/external-db-sync.ts (1)
6-8: LGTM!Simple and clean configuration accessor function.
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts (5)
1-17: LGTM!Imports and test suite setup are correctly structured with
describe.sequentialto avoid shared state issues.
37-60: LGTM!Insert test correctly validates that new users appear in the external DB after sync completes.
113-150: LGTM!Delete test properly validates the full deletion flow including verifying 404 response from the internal API before checking external DB sync.
304-333: LGTM!Resilience test is valuable - it confirms that one bad database configuration doesn't block synchronization to healthy databases.
520-565: LGTM!The race condition protection test cleverly simulates an out-of-order delete by directly issuing a conditional DELETE against the external DB, validating that sequence-based guards work correctly.
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (3)
16-67: LGTM!
TestDbManagerclass is well-designed with proper resource management - unique database names prevent collisions, and cleanup safely handles errors during database drops.
73-88: LGTM!Generic polling helper with configurable timeout, interval, and descriptive error messages - clean and reusable.
93-120: LGTM!
waitForSyncedDataproperly handles the case where the table doesn't exist yet (error code42P01) and supports optional name verification.
| CREATE TABLE "DeletedRow" ( | ||
| "id" UUID NOT NULL, | ||
| "tenancyId" UUID NOT NULL, | ||
| "tableName" TEXT NOT NULL, | ||
| "sequenceId" BIGINT, | ||
| "primaryKey" JSONB NOT NULL, | ||
| "data" JSONB, | ||
| "deletedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "fulfilledAt" TIMESTAMP(3), | ||
|
|
||
| CONSTRAINT "DeletedRow_pkey" PRIMARY KEY ("id") | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the tenancy table definition and schema files
find . -type f \( -name "*.prisma" -o -name "*.sql" \) | head -20Repository: stack-auth/stack-auth
Length of output: 1667
🏁 Script executed:
# Search for Tenancy table references in migration files
rg -i "tenancy|tenant" --type sql | head -30Repository: stack-auth/stack-auth
Length of output: 2870
🏁 Script executed:
# Look for the Prisma schema file
find . -name "schema.prisma" -o -name "*.prisma" | head -10Repository: stack-auth/stack-auth
Length of output: 100
🏁 Script executed:
# Read the Prisma schema to understand Tenancy model
cat ./apps/backend/prisma/schema.prisma | head -100Repository: stack-auth/stack-auth
Length of output: 2728
🏁 Script executed:
# Check the specific migration file mentioned in the review
cat ./apps/backend/prisma/migrations/20251125180000_add_deleted_row_table/migration.sqlRepository: stack-auth/stack-auth
Length of output: 632
🏁 Script executed:
# Search for other tables with tenancyId to see FK patterns
rg "FOREIGN KEY.*tenancyId" apps/backend/prisma/migrations/ | head -10Repository: stack-auth/stack-auth
Length of output: 3153
🏁 Script executed:
# Search for DeletedRow references in the codebase to understand its purpose
rg "DeletedRow" --type-list | head -5Repository: stack-auth/stack-auth
Length of output: 454
🏁 Script executed:
# Search for any Prisma model definition for DeletedRow
rg "model DeletedRow" apps/backend/prisma/schema.prisma -A 20Repository: stack-auth/stack-auth
Length of output: 423
🏁 Script executed:
# Check if there's any recent discussion or comments about this table
rg -i "deleted.*row|sync.*deletion" apps/backend/prisma/migrations/ | grep -i commentRepository: stack-auth/stack-auth
Length of output: 47
🏁 Script executed:
# Search for DeletedRow usage in code files (not just migrations)
find apps/backend -type f \( -name "*.ts" -o -name "*.js" \) -exec rg "DeletedRow" {} + | head -20Repository: stack-auth/stack-auth
Length of output: 47
🏁 Script executed:
# Check the broader migration context - look at surrounding migrations
ls -la apps/backend/prisma/migrations/ | grep "2025" | tail -5Repository: stack-auth/stack-auth
Length of output: 571
🏁 Script executed:
# Check if there are any follow-up migrations that might add the FK
rg "DeletedRow" apps/backend/prisma/migrations/ --type sqlRepository: stack-auth/stack-auth
Length of output: 1667
🏁 Script executed:
# Look at the sync functions to understand DeletedRow purpose
cat apps/backend/prisma/migrations/20251128210004_add_sync_functions/migration.sql | head -100Repository: stack-auth/stack-auth
Length of output: 2821
🏁 Script executed:
# Check for any code comments or documentation files mentioning DeletedRow or sync strategy
find apps/backend -type f \( -name "*.md" -o -name "*.ts" -o -name "*.js" \) -exec grep -l "DeletedRow\|deleted.*row" {} \; 2>/dev/null | head -10Repository: stack-auth/stack-auth
Length of output: 47
🏁 Script executed:
# Look at the trigger that logs to DeletedRow to understand when it's populated
cat apps/backend/prisma/migrations/20251128210003_log_deleted_row_function_and_triggers/migration.sqlRepository: stack-auth/stack-auth
Length of output: 1189
Add foreign key constraint on tenancyId to maintain referential integrity.
The DeletedRow table tracks deletions for external database synchronization but lacks a foreign key constraint on tenancyId, creating orphaned records if a tenancy is deleted. This is inconsistent with other tables in the schema that reference tenancies. Add:
ALTER TABLE "DeletedRow" ADD CONSTRAINT "DeletedRow_tenancyId_fkey" FOREIGN KEY ("tenancyId") REFERENCES "Tenancy"("id") ON DELETE RESTRICT;Using ON DELETE RESTRICT prevents accidental tenancy deletion while there are pending syncs, which is appropriate for this audit log pattern.
🤖 Prompt for AI Agents
In
apps/backend/prisma/migrations/20251125180000_add_deleted_row_table/migration.sql
around lines 1 to 12, the "DeletedRow" table is missing a foreign key on
tenancyId causing possible orphaned records; add a foreign key constraint named
"DeletedRow_tenancyId_fkey" that references "Tenancy"(id) with ON DELETE
RESTRICT to preserve referential integrity and prevent deletion of tenancies
while pending deleted-row syncs exist.
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
Show resolved
Hide resolved
| try { | ||
| await syncExternalDatabases(tenancy); | ||
| } catch (error: any) { | ||
| console.error(` Error syncing external databases for tenant ${tenantId}:`, error); | ||
| } | ||
|
|
||
| return { | ||
| statusCode: 200, | ||
| bodyType: "json", | ||
| body: { | ||
| success: true, | ||
| tenantId, | ||
| timestamp, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Returns success: true even when sync fails.
The handler catches errors from syncExternalDatabases at Lines 52-56 but still returns success: true at Line 61. This could mask sync failures from the caller. Consider returning success: false when an error occurs:
+ let syncSuccess = true;
try {
await syncExternalDatabases(tenancy);
} catch (error: any) {
console.error(` Error syncing external databases for tenant ${tenantId}:`, error);
+ syncSuccess = false;
}
return {
statusCode: 200,
bodyType: "json",
body: {
- success: true,
+ success: syncSuccess,
tenantId,
timestamp,
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await syncExternalDatabases(tenancy); | |
| } catch (error: any) { | |
| console.error(` Error syncing external databases for tenant ${tenantId}:`, error); | |
| } | |
| return { | |
| statusCode: 200, | |
| bodyType: "json", | |
| body: { | |
| success: true, | |
| tenantId, | |
| timestamp, | |
| }, | |
| }; | |
| let syncSuccess = true; | |
| try { | |
| await syncExternalDatabases(tenancy); | |
| } catch (error: any) { | |
| console.error(` Error syncing external databases for tenant ${tenantId}:`, error); | |
| syncSuccess = false; | |
| } | |
| return { | |
| statusCode: 200, | |
| bodyType: "json", | |
| body: { | |
| success: syncSuccess, | |
| tenantId, | |
| timestamp, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In
apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx
around lines 52 to 66, the try/catch logs syncExternalDatabases errors but the
handler always returns success: true; update the handler to return success:
false (and an appropriate error message/status) when an exception is caught:
capture the error in the catch block, set success to false and include error
details (or a sanitized message) and an appropriate HTTP status code in the
returned body so callers can detect failures.
| } | ||
| } | ||
|
|
||
| const rows = await internalPrisma.$queryRawUnsafe<any[]>(fetchQuery, tenancyId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's validation for mapping queries in the config schema
rg -n "internalDbFetchQuery|externalDbUpdateQuery" --type ts -A 3 -B 3Repository: stack-auth/stack-auth
Length of output: 6462
🏁 Script executed:
#!/bin/bash
# Check how mappings are loaded and where they come from
rg -n "externalDbSyncMappings|DB_SYNC_MAPPINGS" --type ts -A 2 -B 2 | head -100Repository: stack-auth/stack-auth
Length of output: 2787
🏁 Script executed:
#!/bin/bash
# Look at the actual query definitions to see how tenancyId is used
cat packages/stack-shared/src/config/db-sync-mappings.ts | head -80Repository: stack-auth/stack-auth
Length of output: 3583
🏁 Script executed:
#!/bin/bash
# Check for any validation logic that ensures queries are safe
rg -n "internalDbFetchQuery|externalDbUpdateQuery" apps/backend/src/lib/external-db-sync.ts -B 5 -A 5Repository: stack-auth/stack-auth
Length of output: 1525
Add validation to ensure mapping queries use parameter placeholders
Mapping configurations (internalDbFetchQuery, externalDbUpdateQuery) are user-configurable but only validated as strings. If users provide custom queries without using parameter placeholders (e.g., $1, $2), SQL injection is possible. Add schema validation or runtime checks to enforce that user-provided queries use parameterized queries for any dynamic values, not string interpolation.
🤖 Prompt for AI Agents
In apps/backend/src/lib/external-db-sync.ts around line 133, the code executes
user-provided SQL via $queryRawUnsafe without verifying that the user-supplied
mapping queries use parameter placeholders; add a runtime validation step (or
extend the configuration schema) before executing fetch/update queries to ensure
any dynamic values use positional parameter placeholders like $1, $2 (e.g., with
a simple regex /\$\d+/) and reject or throw a clear error if placeholders are
absent or inconsistent with expected parameter count; apply this check to both
internalDbFetchQuery and externalDbUpdateQuery, and fail-fast (throw or reject
configuration) rather than executing unsafe raw SQL.
There was a problem hiding this comment.
@N2D4, glad to help! Let me know if you need any clarification on implementing the validation check for the mapping queries. 🐰
🧠 Learnings used
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`
| } catch (error: any) { | ||
| console.error(`Error syncing external DB ${dbId}:`, error); | ||
| } finally { | ||
| await externalClient.end(); | ||
| } |
There was a problem hiding this comment.
Errors are silently swallowed
Sync errors are logged but not propagated. This means:
- Callers cannot detect partial failures
- Monitoring/alerting may miss issues
- The function appears successful even when databases fail to sync
Consider collecting errors and either throwing an aggregate error or returning a result object with success/failure details.
- } catch (error: any) {
- console.error(`Error syncing external DB ${dbId}:`, error);
+ } catch (error: any) {
+ console.error(`Error syncing external DB ${dbId}:`, error);
+ throw error; // Or collect and aggregate errors
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error: any) { | |
| console.error(`Error syncing external DB ${dbId}:`, error); | |
| } finally { | |
| await externalClient.end(); | |
| } | |
| } catch (error: any) { | |
| console.error(`Error syncing external DB ${dbId}:`, error); | |
| throw error; // Or collect and aggregate errors | |
| } finally { | |
| await externalClient.end(); | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/lib/external-db-sync.ts around lines 184 to 188, the catch
block only logs errors and the function always proceeds to finally/return, which
swallows failures; modify the function to capture sync errors and propagate them
by either (a) aggregating errors into a single Error (or a custom
AggregateError) and rethrowing after finally, or (b) changing the function
signature to return a result object that includes per-db success/failure details
and any error messages; ensure externalClient.end() remains in finally, collect
any caught error into a local variable/array, and after the finally block either
throw the aggregated error or return the result object so callers can detect and
act on partial/complete failures.
| expect(res.rows.length).toBe(1); | ||
| expect(res.rows[0].displayName).toBe('Resilience User'); | ||
| }, TEST_TIMEOUT); | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace
Static analysis reports trailing spaces on this line.
-
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🧰 Tools
🪛 GitHub Check: restart-dev-and-test
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
🪛 GitHub Check: restart-dev-and-test-with-custom-base-port
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
🪛 GitHub Check: setup-tests
[warning] 334-334:
Trailing spaces not allowed
[warning] 334-334:
Trailing spaces not allowed
🤖 Prompt for AI Agents
In apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
around line 334 there is trailing whitespace at the end of the line; remove the
extra spaces (trim the line ending) so the file has no trailing space on that
line, and re-run the linter/formatter to ensure no other trailing whitespace
remains.
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts
Outdated
Show resolved
Hide resolved
| UPDATE "OutgoingRequest" | ||
| SET "fulfilledAt" = NOW() | ||
| WHERE "id" IN ( | ||
| SELECT id | ||
| FROM "OutgoingRequest" | ||
| WHERE "fulfilledAt" IS NULL | ||
| ORDER BY "createdAt" | ||
| LIMIT 100 | ||
| FOR UPDATE SKIP LOCKED | ||
| ) | ||
| RETURNING *; | ||
| `; | ||
| return rows; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Outgoing requests are marked as fulfilled before being sent to QStash, so if the HTTP request fails, they're lost forever and never retried.
View Details
📝 Patch Details
diff --git a/apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts b/apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
index dcdaee3b..533e2478 100644
--- a/apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
+++ b/apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
@@ -51,17 +51,12 @@ export const GET = createSmartRouteHandler({
async function claimPendingRequests(): Promise<OutgoingRequest[]> {
return await retryTransaction(globalPrismaClient, async (tx) => {
const rows = await tx.$queryRaw<OutgoingRequest[]>`
- UPDATE "OutgoingRequest"
- SET "fulfilledAt" = NOW()
- WHERE "id" IN (
- SELECT id
- FROM "OutgoingRequest"
- WHERE "fulfilledAt" IS NULL
- ORDER BY "createdAt"
- LIMIT 100
- FOR UPDATE SKIP LOCKED
- )
- RETURNING *;
+ SELECT "id", "createdAt", "qstashOptions", "fulfilledAt"
+ FROM "OutgoingRequest"
+ WHERE "fulfilledAt" IS NULL
+ ORDER BY "createdAt"
+ LIMIT 100
+ FOR UPDATE SKIP LOCKED;
`;
return rows;
});
@@ -92,6 +87,15 @@ export const GET = createSmartRouteHandler({
body: options.body,
});
+ // Only mark as fulfilled after successful QStash publish
+ await retryTransaction(globalPrismaClient, async (tx) => {
+ await tx.$queryRaw`
+ UPDATE "OutgoingRequest"
+ SET "fulfilledAt" = NOW()
+ WHERE "id" = ${request.id};
+ `;
+ });
+
processed++;
} catch (error) {
console.error(
Analysis
Requests marked as fulfilled before QStash publish causes data loss on QStash failures
What fails: claimPendingRequests() marks outgoing requests as fulfilled (sets fulfilledAt) inside a database transaction that commits immediately, before the requests are sent to QStash. If upstash.publishJSON() fails in processRequests(), the error is caught and logged, but the request remains marked as fulfilled in the database and is never retried.
How to reproduce:
- Create a pending OutgoingRequest in the database with
fulfilledAt IS NULL - Configure the poller to call an unreachable or error-prone QStash endpoint
- Call the poller endpoint:
GET /api/latest/internal/external-db-sync/pollerwith valid CRON_SECRET - Observe: upstash.publishJSON() fails, error is logged, but request has
fulfilledAtset - Run poller again - the request is no longer picked up (WHERE clause filters out rows with
fulfilledAt IS NOT NULL)
Result: Outgoing request is permanently lost despite QStash failure. The error log shows the request failed, but there's no way to retry it.
Expected: Only mark requests as fulfilled AFTER successfully sending them to QStash. If QStash fails, the request should remain in pending state and be picked up in the next poller run for retry.
Files modified:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
Fix applied:
- Changed
claimPendingRequests()from UPDATE + RETURNING to SELECT only, removing thefulfilledAt = NOW()update - Moved the
fulfilledAtUPDATE toprocessRequests()after successfulupstash.publishJSON()call - This ensures requests only marked as fulfilled if QStash publish succeeds
| tenantId, | ||
| timestamp, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Bug: Sync engine returns success even when sync fails
The handler catches errors from syncExternalDatabases but still returns success: true in the response regardless of whether the sync succeeded or failed. When an exception is caught at line 54-56, the code logs the error but continues to the return statement which always sets success: true. This means callers (like QStash) cannot distinguish between successful and failed syncs, which could lead to data inconsistencies going undetected.
| `Failed to process outgoing request ${request.id}:`, | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Failed outgoing requests are permanently lost
The claimPendingRequests function atomically marks requests as fulfilled (SET "fulfilledAt" = NOW()) before they're actually processed. If upstash.publishJSON subsequently fails in processRequests, the error is logged but the request is already marked as fulfilled in the database. This causes failed sync requests to be permanently lost with no retry mechanism, potentially leaving external databases out of sync.
| setInterval(() => { | ||
| run(endpoint); | ||
| }, 60000); | ||
| } |
There was a problem hiding this comment.
Bug: Cron jobs don't run immediately on startup
The cron job script uses setInterval without an initial invocation of run(endpoint). This means when the dev server starts, there's a 60-second delay before the sequencer and poller first execute. Any database changes during this initial period won't trigger external sync until after the first interval fires.
N2D4
left a comment
There was a problem hiding this comment.
this looks awesome! i have a lot of comments but overall this is very exciting :]
- i really like how much of this PR is tests (almost two-thirds of all lines of code), made it very pleasant to review :]
- important: every time something is different than what we expect (eg. some assertion is not true, etc) it should always be loud and annoying. the worst thing that could happen is that an error is quiet, bothering users but not appearing in our alerts on sentry. throw new StackAssertionError is the preferred way to do this (or the throwErr function which does the same), with captureError for the cases where that doesn't work.
- good job on separating things into files! this folder structure makes a lot of sense, i think
- make sure to really understand the security sensitive parts of the PR. in particular i think the "grant access to the Users table to everyone" in the table schema query and the custom configs are really dangerous, a less thorough reviewer may have missed them
| $$ LANGUAGE plpgsql; | ||
| -- SPLIT_STATEMENT_SENTINEL | ||
| -- SINGLE_STATEMENT_SENTINEL | ||
| CREATE FUNCTION backfill_null_sequence_ids() |
There was a problem hiding this comment.
this is no longer on pgcron now, right? do we still need this? can't we just have this query in the sequencer endpoint?
apps/backend/prisma/schema.prisma
Outdated
| sequenceId BigInt? @unique | ||
| shouldUpdateSequenceId Boolean @default(true) |
There was a problem hiding this comment.
these fields should be indexed, i think?
apps/backend/prisma/schema.prisma
Outdated
| fulfilledAt DateTime? | ||
| @@index([fulfilledAt]) |
There was a problem hiding this comment.
maybe we can rename this to "startedFulfillingAt" just to make it clearer for the future
apps/backend/prisma/schema.prisma
Outdated
| sequenceId BigInt? @unique | ||
| shouldUpdateSequenceId Boolean @default(true) |
apps/backend/prisma/schema.prisma
Outdated
| sequenceId BigInt? @unique | ||
| shouldUpdateSequenceId Boolean @default(true) |
| ); | ||
| } | ||
|
|
||
| if (newRows.length === 0) return; |
| const placeholderMatches = upsertQuery.match(/\$\d+/g) ?? []; | ||
| const expectedParamCount = | ||
| placeholderMatches.length === 0 | ||
| ? 0 | ||
| : Math.max(...placeholderMatches.map((m) => Number(m.slice(1)))); | ||
|
|
||
| if (expectedParamCount === 0) { | ||
| throw new Error( | ||
| `upsertQuery for table "${tableName}" contains no positional parameters ($1, $2, ...).` + | ||
| ` Your mapping must use parameterized SQL.` | ||
| ); | ||
| } | ||
| const sampleRow = newRows[0]; | ||
| const { tenancyId: _ignore, ...restSample } = sampleRow; | ||
| const orderedKeys = Object.keys(restSample); | ||
|
|
||
| if (orderedKeys.length !== expectedParamCount) { | ||
| throw new Error( | ||
| ` Column count mismatch for table "${tableName}".\n` + | ||
| `→ upsertQuery expects ${expectedParamCount} parameters.\n` + | ||
| `→ internalDbFetchQuery returned ${orderedKeys.length} columns (excluding tenancyId).\n` + | ||
| `Fix your SELECT column order or your SQL parameter order.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
| const placeholderMatches = upsertQuery.match(/\$\d+/g) ?? []; | |
| const expectedParamCount = | |
| placeholderMatches.length === 0 | |
| ? 0 | |
| : Math.max(...placeholderMatches.map((m) => Number(m.slice(1)))); | |
| if (expectedParamCount === 0) { | |
| throw new Error( | |
| `upsertQuery for table "${tableName}" contains no positional parameters ($1, $2, ...).` + | |
| ` Your mapping must use parameterized SQL.` | |
| ); | |
| } | |
| const sampleRow = newRows[0]; | |
| const { tenancyId: _ignore, ...restSample } = sampleRow; | |
| const orderedKeys = Object.keys(restSample); | |
| if (orderedKeys.length !== expectedParamCount) { | |
| throw new Error( | |
| ` Column count mismatch for table "${tableName}".\n` + | |
| `→ upsertQuery expects ${expectedParamCount} parameters.\n` + | |
| `→ internalDbFetchQuery returned ${orderedKeys.length} columns (excluding tenancyId).\n` + | |
| `Fix your SELECT column order or your SQL parameter order.` | |
| ); | |
| } | |
| // Just for our own sanity, make sure that we have the right number of positional parameters | |
| const placeholderMatches = upsertQuery.match(/\$\d+/g) ?? throwErr(`Could not find any positional parameters ($1, $2, ...) in the update SQL query.`);" | |
| const expectedParamCount = Math.max(...placeholderMatches.map((m) => Number(m.slice(1)))); | |
| const sampleRow = newRows[0]; | |
| const orderedKeys = Object.keys(omit(sampleRow, "tenancyId")); | |
| if (orderedKeys.length !== expectedParamCount) { | |
| throw new StackAssertionError(deindent` | |
| Column count mismatch for table ${JSON.stringify(tableName)} | |
| → upsertQuery expects ${expectedParamCount} parameters. | |
| → internalDbFetchQuery returned ${orderedKeys.length} columns (excluding tenancyId). | |
| Fix your SELECT column order or your SQL parameter order. | |
| `); | |
| } |
| rowKeys.every((k, i) => k === orderedKeys[i]); | ||
|
|
||
| if (!validShape) { | ||
| throw new Error( |
There was a problem hiding this comment.
| throw new Error( | |
| throw new StackAssertionError( |
| for (const row of newRows) { | ||
| const { tenancyId, ...rest } = row; | ||
| const rowKeys = Object.keys(rest); | ||
|
|
||
| const validShape = | ||
| rowKeys.length === orderedKeys.length && | ||
| rowKeys.every((k, i) => k === orderedKeys[i]); | ||
|
|
||
| if (!validShape) { | ||
| throw new Error( | ||
| ` Row shape mismatch for table "${tableName}".\n` + | ||
| `Expected column order: [${orderedKeys.join(", ")}]\n` + | ||
| `Received column order: [${rowKeys.join(", ")}]\n` + | ||
| `Your SELECT must be explicit, ordered, and NEVER use SELECT *.\n` + | ||
| `Fix the SELECT in internalDbFetchQuery immediately.` | ||
| ); | ||
| } | ||
| } | ||
| for (const row of newRows) { | ||
| const { tenancyId, ...rest } = row; | ||
| await externalClient.query(upsertQuery, Object.values(rest)); | ||
| } | ||
| } |
There was a problem hiding this comment.
these two for-loops can be merged into one
| for (const row of newRows) { | ||
| const { tenancyId, ...rest } = row; | ||
| const rowKeys = Object.keys(rest); | ||
|
|
||
| const validShape = | ||
| rowKeys.length === orderedKeys.length && | ||
| rowKeys.every((k, i) => k === orderedKeys[i]); | ||
|
|
||
| if (!validShape) { | ||
| throw new Error( | ||
| ` Row shape mismatch for table "${tableName}".\n` + | ||
| `Expected column order: [${orderedKeys.join(", ")}]\n` + | ||
| `Received column order: [${rowKeys.join(", ")}]\n` + | ||
| `Your SELECT must be explicit, ordered, and NEVER use SELECT *.\n` + | ||
| `Fix the SELECT in internalDbFetchQuery immediately.` | ||
| ); | ||
| } | ||
| } | ||
| for (const row of newRows) { | ||
| const { tenancyId, ...rest } = row; | ||
| await externalClient.query(upsertQuery, Object.values(rest)); | ||
| } |
There was a problem hiding this comment.
should we check that the tenancyId is always the same? i think right now we don't do anything with it, right?
Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backend/scripts/run-cron-jobs.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: For blocking alerts and errors, never usetoast, as they are easily missed by the user. Instead, use alerts
Keep hover/click transitions snappy and fast. Don't delay the action with a pre-transition; apply transitions after the action instead
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). UserunAsynchronouslyorrunAsynchronouslyWithAlertinstead for asynchronous error handling
When creating hover transitions, avoid hover-enter transitions and just use hover-exit transitions (e.g.,transition-colors hover:transition-none)
Use ES6 maps instead of records wherever possible
Files:
apps/backend/scripts/run-cron-jobs.ts
🧬 Code graph analysis (1)
apps/backend/scripts/run-cron-jobs.ts (4)
packages/stack-shared/src/utils/env.tsx (1)
getEnvVariable(16-58)apps/dashboard/src/components/vibe-coding/chat-adapters.ts (1)
run(22-59)packages/stack-shared/src/utils/promises.tsx (2)
runAsynchronously(343-366)wait(260-268)packages/stack-shared/src/utils/errors.tsx (1)
StackAssertionError(69-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Vercel Agent Review
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: setup-tests
- GitHub Check: all-good
- GitHub Check: lint_and_build (latest)
- GitHub Check: docker
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: restart-dev-and-test
…er/route.ts Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
…er/route.ts Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts (3)
46-49: Consider renamingbusySleepMsand revisiting the 2‑minute windowMinor/naming and tuning nit:
busySleepMsis a bit misleading since this is not a busy-spin; something likepollSleepMsorloopSleepMswould be clearer.- Previous feedback suggested a
maxDurationMsof ~3 minutes to tolerate worst‑case Vercel cron skew (e.g. invocations at:05and:06still overlapping enough to drain backlogs). If that concern still applies, consider bumping from 2 to 3 minutes.
79-85: Document thehost.docker.internalrewrite for dev/testThe dev/test rewrite of
localhost/127.0.0.1tohost.docker.internalis subtle and environment‑specific. Adding a short comment here explaining that this is required for Dockerized e2e / local QStash to reach the backend from inside the container would help future readers understand why this block exists and avoid accidentally “simplifying” it away.You might also cache
getNodeEnvironment()in a local variable to avoid calling it twice, though that’s purely cosmetic.
51-68: Avoid markingOutgoingRequestas fulfilled before QStash publish succeedsRight now
claimPendingRequestssetsfulfilledAt = NOW()in a transaction (lines 54-64), beforeprocessRequestsactually callsupstash.publishJSON(lines 88-91). IfpublishJSONfails, the request stays marked as fulfilled and is never retried, so the external DB can silently fall out of sync. This matches prior automated feedback and should be treated as at-least-once delivery instead of maybe-once.A safer pattern here is:
- In
claimPendingRequests, only select pending rows withFOR UPDATE SKIP LOCKEDand do not touchfulfilledAt.- After a successful
publishJSONcall for a given request, setfulfilledAt = NOW()in a separate transaction.- Leave failed publishes with
fulfilledAt IS NULLso they get picked up again on the next poll.Concretely:
- async function claimPendingRequests(): Promise<OutgoingRequest[]> { - return await retryTransaction(globalPrismaClient, async (tx) => { - const rows = await tx.$queryRaw<OutgoingRequest[]>` - UPDATE "OutgoingRequest" - SET "fulfilledAt" = NOW() - WHERE "id" IN ( - SELECT id - FROM "OutgoingRequest" - WHERE "fulfilledAt" IS NULL - ORDER BY "createdAt" - LIMIT 100 - FOR UPDATE SKIP LOCKED - ) - RETURNING *; - `; - return rows; - }); - } + async function claimPendingRequests(): Promise<OutgoingRequest[]> { + return await retryTransaction(globalPrismaClient, async (tx) => { + const rows = await tx.$queryRaw<OutgoingRequest[]>` + SELECT "id", "createdAt", "qstashOptions", "fulfilledAt" + FROM "OutgoingRequest" + WHERE "fulfilledAt" IS NULL + ORDER BY "createdAt" + LIMIT 100 + FOR UPDATE SKIP LOCKED; + `; + return rows; + }); + }and after a successful publish:
- await upstash.publishJSON({ - url: fullUrl, - body: options.body, - }); - - processed++; + await upstash.publishJSON({ + url: fullUrl, + body: options.body, + }); + + // Only mark as fulfilled after successful QStash publish + await retryTransaction(globalPrismaClient, async (tx) => { + await tx.$queryRaw` + UPDATE "OutgoingRequest" + SET "fulfilledAt" = NOW() + WHERE "id" = ${request.id}; + `; + }); + + processed++;This removes the permanent-loss failure mode; downstream code should already be idempotent given the sequencing/sync use case, but if not, please double-check that repeated publishes are acceptable.
Also applies to: 88-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: For blocking alerts and errors, never usetoast, as they are easily missed by the user. Instead, use alerts
Keep hover/click transitions snappy and fast. Don't delay the action with a pre-transition; apply transitions after the action instead
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). UserunAsynchronouslyorrunAsynchronouslyWithAlertinstead for asynchronous error handling
When creating hover transitions, avoid hover-enter transitions and just use hover-exit transitions (e.g.,transition-colors hover:transition-none)
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
apps/backend/src/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The project uses a custom route handler system in the backend for consistent API responses
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
apps/{backend,dashboard}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if you can avoid them. Instead, prefer using a client component and prefer
usePathnameinstead ofawait params
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
🧠 Learnings (3)
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to apps/backend/src/app/api/**/*.{ts,tsx} : The project uses a custom route handler system in the backend for consistent API responses
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to **/*.{ts,tsx} : NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). Use `runAsynchronously` or `runAsynchronouslyWithAlert` instead for asynchronous error handling
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to **/*.{ts,tsx} : For blocking alerts and errors, never use `toast`, as they are easily missed by the user. Instead, use alerts
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: docker
- GitHub Check: setup-tests
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: all-good
- GitHub Check: Vercel Agent Review
- GitHub Check: restart-dev-and-test
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/backend/scripts/run-cron-jobs.ts (1)
25-33: Cron jobs don't execute immediately on startup.The loop waits before the first execution, meaning there's a delay of up to 120 seconds before the first sync triggers. Consider running the endpoint once before entering the loop if immediate execution is desired.
for (const endpoint of endpoints) { runAsynchronously(async () => { + run(endpoint); // Initial immediate execution while (true) { - run(endpoint); // Vercel only guarantees minute-granularity for cron jobs, so we randomize the interval await wait(Math.random() * 120_000); + run(endpoint); } }); }apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts (1)
51-68: Critical: Claimed requests are lost if QStash publish fails.The
claimPendingRequestsfunction setsstartedFulfillingAt = NOW()before processing. Ifupstash.publishJSONfails (lines 90-93), the request remains claimed (startedFulfillingAtis set) and will never be retried since the WHERE clause filters forstartedFulfillingAt IS NULL.Consider either:
- Adding a
fulfilledAtcolumn that's only set on success, with retry logic for stalestartedFulfillingAt- Resetting
startedFulfillingAtto NULL on failureasync function claimPendingRequests(): Promise<OutgoingRequest[]> { return await retryTransaction(globalPrismaClient, async (tx) => { const rows = await tx.$queryRaw<OutgoingRequest[]>` UPDATE "OutgoingRequest" SET "startedFulfillingAt" = NOW() WHERE "id" IN ( SELECT id FROM "OutgoingRequest" - WHERE "startedFulfillingAt" IS NULL + WHERE "startedFulfillingAt" IS NULL + OR ("startedFulfillingAt" < NOW() - INTERVAL '5 minutes' AND "fulfilledAt" IS NULL) ORDER BY "createdAt" LIMIT 100 FOR UPDATE SKIP LOCKED ) RETURNING *; `; return rows; }); }And after successful publish:
// After upstash.publishJSON succeeds await globalPrismaClient.$executeRaw` UPDATE "OutgoingRequest" SET "fulfilledAt" = NOW() WHERE "id" = ${request.id} `;apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (1)
334-337: Hardcoded database credentials should be externalized.This concern was previously flagged. The password should be read from environment variables rather than hardcoded in source.
🧹 Nitpick comments (6)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (1)
345-394: Consider reducing the 70-second sleep duration.The 70-second sleep makes this test very slow. While it's needed to ensure the poller runs during the transaction, consider whether a shorter duration (e.g., 15-20 seconds) would still validate the behavior while improving test runtime.
- await sleep(70000); + await sleep(20000);apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts (1)
16-92: Consider parallelizing tenant sync enqueueing.The sequential
awaitcalls in theforloops (lines 39-41, 65-67, 89-91) process tenants one at a time. SinceenqueueTenantSyncoperations are independent, you could batch them withPromise.allfor better throughput during high-volume backfills.However, given this runs continuously in a loop, the sequential approach may be intentional to avoid overwhelming the database. This is a minor optimization opportunity.
- for (const { tenancyId } of projectUserTenants) { - await enqueueTenantSync(tenancyId); - } + await Promise.all(projectUserTenants.map(({ tenancyId }) => enqueueTenantSync(tenancyId)));apps/backend/src/lib/external-db-sync.ts (1)
182-190: Consider returning sync results for observability.Errors are captured but the function returns
void, making it impossible for callers to know if any databases failed to sync. This was flagged in past reviews. Consider returning a result object with per-database success/failure status for better observability and potential retry logic:-export async function syncExternalDatabases(tenancy: Tenancy) { +export async function syncExternalDatabases(tenancy: Tenancy): Promise<{ results: Map<string, { success: boolean; error?: unknown }> }> { const externalDatabases = tenancy.config.dbSync.externalDatabases; const internalPrisma = await getPrismaClientForTenancy(tenancy); + const results = new Map<string, { success: boolean; error?: unknown }>(); for (const [dbId, dbConfig] of Object.entries(externalDatabases)) { try { await syncDatabase(dbId, dbConfig, internalPrisma, tenancy.id); + results.set(dbId, { success: true }); } catch (error) { // Log the error but continue syncing other databases // This ensures one bad database config doesn't block successful syncs to other databases captureError(`external-db-sync-${dbId}`, error); + results.set(dbId, { success: false, error }); } } + return { results }; }Based on learnings, this also uses ES6 Map as preferred by coding guidelines.
apps/backend/prisma/migrations/20251125030551_external_db_sync/migration.sql (3)
1-8: Clarify the rationale forINCREMENT BY 11.The sequence increments by 11 which is unusual. While this works, the choice seems arbitrary. Common reasons for non-1 increments include:
- Reserving gaps for manual insertions
- Distributing load across shards
- Supporting interleaved sequences
Consider adding a comment explaining this design choice, or simplify to
INCREMENT BY 1if there's no specific requirement.
46-57: Consider adding a foreign key constraint or documenting the lack thereof.The
DeletedRow.tenancyIdreferences a tenancy but has no foreign key constraint. This is likely intentional (to preserve deletion records even if the tenancy is deleted), but worth documenting. If tenancy deletion should cascade toDeletedRow, add the FK constraint.
132-172: Document thetenancyIdrequirement for tables usinglog_deleted_row.The trigger function assumes all tracked tables have a
tenancyIdcolumn (line 162), which currently holds true for bothProjectUserandContactChannel. However, if future migrations add this trigger to a table withouttenancyId, it will fail at runtime. Consider adding a comment in the migration documenting this requirement, or implement a defensive check within the function (e.g., checking column existence before accessingOLD."tenancyId").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/backend/package.json(3 hunks)apps/backend/prisma/migrations/20251125030551_external_db_sync/migration.sql(1 hunks)apps/backend/prisma/schema.prisma(7 hunks)apps/backend/scripts/run-cron-jobs.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts(1 hunks)apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx(1 hunks)apps/backend/src/lib/external-db-sync.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-high-volume.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts(1 hunks)apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts(1 hunks)docker/dependencies/docker.compose.yaml(1 hunks)packages/stack-shared/src/config/db-sync-mappings.ts(1 hunks)packages/stack-shared/src/config/schema-fuzzer.test.ts(1 hunks)packages/stack-shared/src/config/schema.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/stack-shared/src/config/schema-fuzzer.test.ts
- docker/dependencies/docker.compose.yaml
- apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: For blocking alerts and errors, never usetoast, as they are easily missed by the user. Instead, use alerts
Keep hover/click transitions snappy and fast. Don't delay the action with a pre-transition; apply transitions after the action instead
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). UserunAsynchronouslyorrunAsynchronouslyWithAlertinstead for asynchronous error handling
When creating hover transitions, avoid hover-enter transitions and just use hover-exit transitions (e.g.,transition-colors hover:transition-none)
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/lib/external-db-sync.tspackages/stack-shared/src/config/db-sync-mappings.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-high-volume.test.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/scripts/run-cron-jobs.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.tspackages/stack-shared/src/config/schema.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
apps/backend/src/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The project uses a custom route handler system in the backend for consistent API responses
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
apps/{backend,dashboard}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if you can avoid them. Instead, prefer using a client component and prefer
usePathnameinstead ofawait params
Files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/lib/external-db-sync.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When writing tests, prefer .toMatchInlineSnapshot over other selectors, if possible
Files:
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-high-volume.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
packages/stack-shared/src/config/schema.ts
📄 CodeRabbit inference engine (AGENTS.md)
Whenever making backwards-incompatible changes to the config schema, update the migration functions in
packages/stack-shared/src/config/schema.ts
Files:
packages/stack-shared/src/config/schema.ts
🧠 Learnings (7)
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to apps/backend/src/app/api/**/*.{ts,tsx} : The project uses a custom route handler system in the backend for consistent API responses
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to **/*.{ts,tsx} : NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error). Use `runAsynchronously` or `runAsynchronouslyWithAlert` instead for asynchronous error handling
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/lib/external-db-sync.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to **/*.{ts,tsx} : For blocking alerts and errors, never use `toast`, as they are easily missed by the user. Instead, use alerts
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts
📚 Learning: 2025-10-11T04:13:19.308Z
Learnt from: N2D4
Repo: stack-auth/stack-auth PR: 943
File: examples/convex/app/action/page.tsx:23-28
Timestamp: 2025-10-11T04:13:19.308Z
Learning: In the stack-auth codebase, use `runAsynchronouslyWithAlert` from `stackframe/stack-shared/dist/utils/promises` for async button click handlers and form submissions instead of manual try/catch blocks. This utility automatically handles errors and shows alerts to users.
Applied to files:
apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.tsapps/backend/src/lib/external-db-sync.tsapps/backend/scripts/run-cron-jobs.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`
Applied to files:
apps/backend/src/lib/external-db-sync.tspackages/stack-shared/src/config/db-sync-mappings.tspackages/stack-shared/src/config/schema.tsapps/backend/prisma/schema.prismaapps/backend/package.json
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to **/*.{ts,tsx} : Use ES6 maps instead of records wherever possible
Applied to files:
apps/backend/src/lib/external-db-sync.ts
📚 Learning: 2025-11-28T21:21:39.123Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Always add new E2E tests when changing the API or SDK interface
Applied to files:
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
🧬 Code graph analysis (5)
apps/backend/src/lib/external-db-sync.ts (6)
packages/stack-shared/src/utils/errors.tsx (2)
throwErr(10-19)StackAssertionError(69-85)packages/stack-shared/src/config/db-sync-mappings.ts (1)
DEFAULT_DB_SYNC_MAPPINGS(1-165)apps/backend/src/prisma-client.tsx (2)
PrismaClientTransaction(22-22)getPrismaClientForTenancy(68-70)packages/stack-shared/src/config/schema.ts (1)
CompleteConfig(1092-1092)packages/stack-shared/src/utils/results.tsx (1)
error(36-41)apps/backend/src/lib/tenancies.tsx (1)
Tenancy(53-53)
apps/backend/scripts/run-cron-jobs.ts (3)
packages/stack-shared/src/utils/env.tsx (1)
getEnvVariable(16-58)packages/stack-shared/src/utils/promises.tsx (2)
runAsynchronously(343-366)wait(260-268)packages/stack-shared/src/utils/errors.tsx (1)
StackAssertionError(69-85)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (1)
apps/e2e/tests/helpers.ts (2)
niceFetch(161-185)STACK_BACKEND_BASE_URL(284-284)
packages/stack-shared/src/config/schema.ts (1)
packages/stack-shared/src/schema-fields.ts (4)
yupObject(247-251)yupRecord(283-322)userSpecifiedIdSchema(442-442)yupString(187-190)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts (1)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (8)
TestDbManager(34-118)createProjectWithExternalDb(271-284)waitForSyncedData(154-181)TEST_TIMEOUT(11-11)waitForSyncedDeletion(186-206)verifyNotInExternalDb(235-238)waitForTable(211-230)waitForCondition(124-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Vercel Agent Review
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: build (22.x)
- GitHub Check: all-good
- GitHub Check: setup-tests
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: docker
- GitHub Check: check_prisma_migrations (22.x)
🔇 Additional comments (61)
apps/backend/package.json (1)
10-10: LGTM! Dev script and cron-jobs integration.The new
run-cron-jobsscript is properly wired into the dev workflow, allowing local simulation of Vercel cron triggers for external DB sync.Also applies to: 38-39
apps/backend/scripts/run-cron-jobs.ts (1)
1-3: Import issue resolved.The
waitfunction is now correctly imported from@stackframe/stack-shared/dist/utils/promises.packages/stack-shared/src/config/schema.ts (2)
203-211: LGTM! External database sync configuration schema.The schema properly restricts user configuration to only connection details (
type,connectionString) without exposing internal SQL mappings. TheyupRecordwithuserSpecifiedIdSchemaensures valid external database identifiers.
579-585: LGTM! Organization config defaults for dbSync.The defaults correctly provide a factory function for dynamic external database entries with
type: undefinedandconnectionString: undefined, aligning with the schema structure.packages/stack-shared/src/config/db-sync-mappings.ts (3)
96-162: LGTM! Robust upsert pattern with metadata tracking.The CTE-based approach properly handles:
- Deletion via the
deletedCTE- Upsert via
INSERT ... ON CONFLICT DO UPDATE- Sequence tracking with
GREATESTto handle out-of-order processing- Atomic metadata update in single transaction
19-20: REVOKE/GRANT statements are idempotent—no action needed.The REVOKE ALL and GRANT SELECT statements execute every sync operation, but this is safe. PostgreSQL treats both statements as idempotent: running them repeatedly produces the same result as running them once, with no errors or conflicts regardless of prior role configurations. This is the standard and intended behavior.
Likely an incorrect or invalid review comment.
36-56: Indexes are already in place for ContactChannel subqueries.The UNIQUE INDEX
ContactChannel_tenancyId_projectUserId_type_isPrimary_keyexists and covers all filter columns (tenancyId,projectUserId,type,isPrimary) used in both correlated subqueries. No additional indexing is needed.apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts (3)
34-68: LGTM! Well-designed TestDbManager class.The class properly:
- Uses ES6 Map for database tracking (per coding guidelines)
- Applies connection timeouts to prevent leaks
- Generates unique database names to avoid conflicts
- Tracks both logical names and actual database names separately
70-117: Robust cleanup with graceful error handling.The cleanup sequence properly:
- Clears project configs first to stop sync cron
- Closes clients with timeout fallback
- Terminates lingering connections before dropping databases
- Handles errors gracefully without failing the entire cleanup
136-143: Good resilience for connection errors in polling.Catching specific PostgreSQL error codes (
57P01,08006,53300) and retrying is appropriate for test utilities that may encounter transient connection issues during concurrent test execution.apps/backend/src/app/api/latest/internal/external-db-sync/poller/route.ts (3)
13-13: Import issue resolved.The
captureErrorfunction is now correctly imported from@stackframe/stack-shared/dist/utils/errors.
79-87: Good: localhost replacement for Docker networking.The comment explains why localhost/127.0.0.1 is replaced with
host.docker.internalin dev/test environments where QStash runs in Docker.
96-101: Good error handling pattern.Using
captureErrorinstead ofconsole.errorensures errors are properly tracked and alerts are triggered, following the project's coding guidelines.apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-high-volume.test.ts (3)
1-14: LGTM!Imports are well-organized and all used symbols are referenced in the test.
16-27: LGTM!Test suite setup follows the established pattern with proper initialization and cleanup with appropriate timeout.
39-176: High-volume pagination test is well-structured.The test correctly exercises the >1000 row pagination path with:
- Direct SQL batch inserts for performance
- Appropriate 8-minute timeout for large data sync
- Spot-checks at boundaries (first, middle, last)
- Proper resource cleanup in finally block
One minor observation: the
console.logstatements are helpful for debugging but could be noisy in CI. Consider whether these should remain or be wrapped in a debug flag.apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts (14)
1-18: LGTM!Imports are clean and organized. The previously flagged unused
generateSecureRandomStringimport has been removed.
20-31: LGTM!Standard test suite setup with proper lifecycle management.
40-158: Multi-tenant isolation test is thorough.Good coverage of cross-project data isolation with:
- Two projects with different DB configurations (2 DBs vs 3 DBs)
- Verification that each user only appears in their own project's databases
- Explicit negative assertions (user A not in project B's DBs and vice versa)
168-242: Metadata tracking test validates sync progress correctly.Properly verifies that
_stack_sync_metadatatable trackslast_synced_sequence_idand advances after new syncs.
252-297: Idempotency test is well-designed.Confirms that re-running sync doesn't duplicate rows and preserves existing row IDs.
307-332: Special characters test covers important edge cases.Tests emoji, quotes, and international characters (Chinese) to validate encoding safety.
342-438: High volume batching test is solid.Similar structure to the high-volume file but with 200 users - good for faster CI runs while still exercising batching.
448-548: Complex sequence test validates interleaved operations.Good coverage of update → delete → insert sequences with proper state verification.
558-625: Write protection test is important for security.Validates that readonly credentials cannot modify data. The dynamic role creation and permission testing is well-structured.
635-675: Multiple updates before sync test validates last-write-wins semantics.Correctly verifies that only the final update state is exported.
685-727: Delete before first sync test is a valuable edge case.Ensures records deleted before initial sync never appear externally.
737-804: Re-create email after delete test validates proper handling of recycled identifiers.Good verification that new users with previously-deleted emails get fresh IDs.
814-905: Complex lifecycle test is comprehensive.Exercises the full create → update → update → delete → create → update path with proper assertions at each stage.
915-1111: High volume with deletes interleaved test validates batch consistency.Good coverage of mixed insert/delete operations with proper verification of final dataset integrity.
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-race.test.ts (9)
1-18: LGTM!Imports and sleep helper are appropriate for race condition testing.
20-31: LGTM!Standard test suite setup.
40-78: Concurrent sync test validates idempotency under parallel updates.Good test for ensuring overlapping pollers don't cause duplicates or data corruption.
88-127: Delete-after-update test validates delete precedence.Properly verifies that delete events win over closely preceding updates.
137-261: Pagination boundary delete test is well-structured.Tests that deletions aren't skipped due to LIMIT boundaries with 300 users.
401-464: Transaction commit test is well-designed.Validates that committed transactions are picked up by the poller.
472-542: Second uncommitted transaction test validates isolation.Good coverage of the scenario where T1 is committed but T2 remains uncommitted.
558-666: Out-of-order commits test is excellent.Tests a complex scenario with two different rows and interleaved commits to validate visibility rules.
676-769: Sequential updates lifecycle test rounds out the coverage.Validates end-to-end flow with multiple sequential commits.
apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts (3)
1-12: LGTM!Imports are appropriate for the route handler functionality.
94-114: Duplicate prevention logic is sound.The
NOT EXISTScheck prevents duplicate sync requests for the same tenant. The use ofgen_random_uuid()and proper JSON construction is correct.
116-175: Route handler is well-structured.Good implementation following the project's custom route handler system with:
- Proper Bearer token authentication against CRON_SECRET
- Bounded execution time (3 minutes)
- Error capture without crashing the loop
- Appropriate response schema
The 50ms poll interval provides good responsiveness while not being too aggressive.
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts (11)
1-14: LGTM!Imports are clean and all symbols are used.
16-27: LGTM!Standard test suite setup with proper lifecycle management.
37-60: Insert test is well-structured.Clean test for basic user creation and sync verification.
70-103: Update test validates change propagation.Good coverage of the update flow with before/after verification.
113-150: Delete test is comprehensive.Includes both API-level 404 verification and external DB removal check.
160-205: Sync mechanism verification test validates timing guarantees.Good test that verifies data only appears after sync by checking table existence before configuration.
215-256: Full CRUD lifecycle test is a solid integration test.Exercises the complete create → update → delete flow with proper assertions.
266-297: Automatic table creation test validates schema provisioning.Good verification that the sync engine creates necessary tables on demand.
307-336: Resilience test is important for production reliability.Validates that one failing DB connection doesn't block successful targets. This is critical for graceful degradation.
347-390: Multi-email test validates user-centric schema correctly.Good coverage ensuring only one row per user with primary email populated.
400-446: Metadata tracking test validates sync progress tracking.Properly verifies
_stack_sync_metadatatable updates after each sync.apps/backend/prisma/schema.prisma (4)
174-176: LGTM!The
sequenceIdandshouldUpdateSequenceIdfields are correctly added toProjectUserwith appropriate unique constraint and index for the sync backfill workflow.
257-259: LGTM!The
sequenceIdandshouldUpdateSequenceIdfields are correctly added toContactChannel, mirroring theProjectUserpattern for consistent change tracking.
885-894: LGTM!The
OutgoingRequestmodel is well-structured for queuing external sync requests. The composite index on(startedFulfillingAt, createdAt)supports efficient polling for pending requests ordered by creation time.
896-916: LGTM!The
DeletedRowmodel correctly captures deletion events with:
- Proper tenant isolation via
tenancyId- Global ordering via unique
sequenceId- Composite index
(tenancyId, tableName, sequenceId)for efficient querying per tenant/table- Partial index on
shouldUpdateSequenceIdfor backfill queriesThis addresses the past review feedback about indexing.
apps/backend/src/lib/external-db-sync.ts (2)
9-64: LGTM!The
pushRowsToExternalDbfunction has solid validation:
- Parameter count validation against upsert query placeholders
- Tenancy alignment check to prevent cross-tenant data mixing
- Row shape validation ensuring explicit, ordered columns
Good defensive programming that will catch mapping configuration errors early.
94-95: No SQL injection risk fromDEFAULT_DB_SYNC_MAPPINGSoverrides.The hardcoded
DEFAULT_DB_SYNC_MAPPINGScannot be overridden by tenant configuration—the codebase explicitly states "users cannot customize mappings" at line 154. The mapping is always sourced fromObject.entries(DEFAULT_DB_SYNC_MAPPINGS)at line 156, and no alternative mapping mechanism exists. Additionally,tenancyIdandlastSequenceIdare correctly passed as separate arguments to$queryRawUnsafe, ensuring proper parameter binding rather than string interpolation.apps/backend/prisma/migrations/20251125030551_external_db_sync/migration.sql (2)
83-91: LGTM! Partial indexes are correctly defined.The partial indexes with
WHERE "shouldUpdateSequenceId" = TRUEare efficient for the backfill query pattern, as they only index rows that need processing. This is a good optimization.
97-126: LGTM!The
reset_sequence_id_on_updatefunction and triggers are well-designed:
- Only triggers when
shouldUpdateSequenceId = FALSEto avoid redundant updates- Uses
BEFORE UPDATEto modify the row before it's written- Correctly marks rows for re-sync after any change
| try { | ||
| await externalClient.connect(); | ||
|
|
||
| // Always use DEFAULT_DB_SYNC_MAPPINGS - users cannot customize mappings | ||
| // because internalDbFetchQuery runs against Stack Auth's internal DB | ||
| for (const [mappingId, mapping] of Object.entries(DEFAULT_DB_SYNC_MAPPINGS)) { | ||
| await syncMapping( | ||
| externalClient, | ||
| mappingId, | ||
| mapping, | ||
| internalPrisma, | ||
| dbId, | ||
| tenancyId, | ||
| dbConfig.type, | ||
| ); | ||
| } | ||
|
|
||
| } catch (error) { | ||
| await externalClient.end(); | ||
| captureError(`external-db-sync-${dbId}`, error); | ||
| return; | ||
| } | ||
|
|
||
| await externalClient.end(); | ||
| } |
There was a problem hiding this comment.
Connection leak on error path.
If an error occurs after externalClient.connect() but before the catch block completes, and externalClient.end() itself throws, the connection may not be properly closed. Consider using try/finally to ensure cleanup:
+ let connected = false;
try {
await externalClient.connect();
+ connected = true;
// Always use DEFAULT_DB_SYNC_MAPPINGS - users cannot customize mappings
// because internalDbFetchQuery runs against Stack Auth's internal DB
for (const [mappingId, mapping] of Object.entries(DEFAULT_DB_SYNC_MAPPINGS)) {
await syncMapping(
externalClient,
mappingId,
mapping,
internalPrisma,
dbId,
tenancyId,
dbConfig.type,
);
}
-
} catch (error) {
- await externalClient.end();
captureError(`external-db-sync-${dbId}`, error);
- return;
+ } finally {
+ if (connected) {
+ await externalClient.end();
+ }
}
-
- await externalClient.end();
}🤖 Prompt for AI Agents
In apps/backend/src/lib/external-db-sync.ts around lines 151 to 175, the current
try/catch can leak the externalClient connection if externalClient.end() itself
throws or if an exception occurs after connect() but before the catch completes;
change the structure to use try { await externalClient.connect(); ...await
syncMapping(...) } finally { try { await externalClient.end(); } catch (endErr)
{ captureError(`external-db-sync-${dbId}-close`, endErr); } } and keep the
existing captureError for the main error path inside the try block so that sync
errors are reported while guaranteeing the client is always closed.
This PR revolves around the following components
What has been done
How to review this PR:
Note
Introduces a cron-driven external DB sync pipeline with global sequencing, internal poller and webhook sync engine, new DB tables/functions, config schema/mappings, and comprehensive e2e tests.
global_seq_id) andsequenceId/shouldUpdateSequenceIdtoProjectUser,ContactChannel,DeletedRowwith partial indexes.DeletedRow(capture deletes) andOutgoingRequest(queue) tables; add unique/indexes.log_deleted_row,reset_sequence_id_on_update,backfill_null_sequence_ids,enqueue_tenant_sync.GET /api/latest/internal/external-db-sync/sequencer,GET /poller,POST /sync-engine(Upstash-verified) for sync orchestration.vercel.jsonschedules and localscripts/run-cron-jobs.ts; start in dev viadevscript.src/lib/external-db-sync.tsto read tenant mappings and upsert to external Postgres (schema bootstrap, param checks, sequencing).DEFAULT_DB_SYNC_MAPPINGSand config schemadbSync.externalDatabasesin shared config.external-db-testPostgres for tests; e2e deps forpgtypes.Written by Cursor Bugbot for commit 3f2a8ef. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.