various new clickhouse syncs#1267
Closed
BilalG1 wants to merge 24 commits into
Closed
Conversation
…c-session-replays
…ck-auth/stack-auth into clickhouse-sync-extra-tables
…invites' into clickhouse-sync-email-outbox-table # Conflicts: # apps/backend/scripts/clickhouse-migrations.ts # apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts # apps/backend/src/lib/external-db-sync.ts # apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts # apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts
…le' into clickhouse-sync-session-replays
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds sequenceId and shouldUpdateSequenceId fields and indexes to multiple models, extends external-db-sync to cover teams/permissions/email outbox/session replays/refresh tokens/oauth accounts/verification codes, updates sequencer/status/routes, ClickHouse mappings/migrations, CRUD handlers to record sync events, and extensive e2e tests and migration scripts. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Backend as Backend API
participant Prisma as Prisma Client
participant DeletedRow as DeletedRow Table
participant Sequencer as Sync Sequencer
participant ClickHouse as ClickHouse
participant ExtDB as External Postgres
Client->>Backend: Create/Update entity (e.g., Team) with upsert
Backend->>Prisma: upsert/create wrapped by withExternalDbSyncUpdate(...)
Prisma-->>Backend: Return row (may include sequenceId/flags)
alt Deletion path
Client->>Backend: Delete entity
Backend->>DeletedRow: recordExternalDbSyncDeletion(...) (insert DeletedRow)
Backend->>Prisma: delete entity
Prisma-->>Backend: Confirm deletion
end
Sequencer->>Prisma: SELECT ... FOR UPDATE SKIP LOCKED (shouldUpdateSequenceId = TRUE)
Prisma-->>Sequencer: Batch of rows
Sequencer->>Prisma: UPDATE set sequenceId = nextval(...), shouldUpdateSequenceId = FALSE
Sequencer->>DeletedRow: include deleted events in batch
Sequencer->>ClickHouse: fetch internal rows by sequence range
ClickHouse-->>Sequencer: Return rows
Sequencer->>ExtDB: Execute CTE upsert/delete to apply batch
ExtDB-->>Sequencer: Confirm and update _stack_sync_metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR improves the ClickHouse sync speed by removing the Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Sync as syncClickhouseMapping
participant CH as ClickHouse
Note over Sync,CH: Before this PR (per mapping, per invocation)
Sync->>CH: ensureClickhouseSchema (CREATE TABLE IF NOT EXISTS — DDL round trip)
Sync->>CH: getClickhouseLastSyncedSequenceId
loop Batch loop
Sync->>CH: pushRowsToClickhouse (INSERT)
Sync->>CH: updateClickhouseSyncMetadata
end
Note over Sync,CH: After this PR (DDL call removed)
Sync->>CH: getClickhouseLastSyncedSequenceId
loop Batch loop
Sync->>CH: pushRowsToClickhouse (INSERT)
Sync->>CH: updateClickhouseSyncMetadata
end
|
N2D4
approved these changes
Mar 21, 2026
N2D4
approved these changes
Mar 21, 2026
N2D4
approved these changes
Mar 21, 2026
N2D4
changed the base branch from
clickhouse-sync-sessions-and-connected-accounts
to
dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/backend/src/app/api/latest/users/crud.tsx (1)
1193-1203:⚠️ Potential issue | 🟠 MajorWrap the refresh-token sync write and
deleteMany()together.These tombstones are recorded before the delete and outside any transaction. If
projectUserRefreshToken.deleteMany()fails, external-db-sync will emit deletions for tokens that are still present; please execute both calls inside oneglobalPrismaClient.$transaction(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/users/crud.tsx` around lines 1193 - 1203, Wrap the tombstone write and the actual deletion in a single transaction so the external-db-sync deletions are only emitted if the DB delete succeeds: move the calls to recordExternalDbSyncRefreshTokenDeletionsForUser(...) and globalPrismaClient.projectUserRefreshToken.deleteMany(...) into a single globalPrismaClient.$transaction(...) and run them there (ensure you pass both operations as transactional actions so they either both commit or both roll back). Make sure to preserve the same parameters (auth.tenancy.id and params.user_id) and return/await the transaction result as appropriate.apps/backend/src/app/api/latest/auth/sessions/current/route.tsx (1)
37-48:⚠️ Potential issue | 🔴 CriticalDon't record the refresh-token tombstone before the delete outside a transaction.
recordExternalDbSyncDeletion()throws when it inserts 0DeletedRowrows forProjectUserRefreshToken, so an already-missing token now fails here beforedeleteMany()can translate it toKnownErrors.RefreshTokenNotFoundOrExpired(). It also commits the tombstone before the actual delete, so a later delete failure leaves external-db-sync ahead of the source DB. Please run both operations in oneglobalPrismaClienttransaction and preserve the 0-row mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/auth/sessions/current/route.tsx` around lines 37 - 48, Run the delete and the tombstone insertion inside a single Prisma transaction so the tombstone cannot be committed without the source delete: wrap globalPrismaClient.projectUserRefreshToken.deleteMany(...) and the call to recordExternalDbSyncDeletion(...) in a single globalPrismaClient.$transaction(...) and ensure you pass the delete result (the deleted count) or an explicit expected-deleted-count to recordExternalDbSyncDeletion so it can insert the 0-row mapping instead of throwing; if needed, update recordExternalDbSyncDeletion to accept an expectedCount/allowZero flag and avoid throwing when expectedCount === 0 while still inserting the corresponding DeletedRow record for tenancy.id and refreshTokenId.apps/backend/src/lib/permissions.tsx (2)
433-460:⚠️ Potential issue | 🟠 MajorRecord team-permission tombstones before
deleteMany.The project-scope branch now records external deletions, but the team-scope branch still bulk-deletes
TeamMemberDirectPermissionrows directly. Deleting a team permission definition will therefore leave staleteam_permissionsrows in external DBs.Possible fix
if (options.scope === "team") { + const teamPermissions = await sourceOfTruthTx.teamMemberDirectPermission.findMany({ + where: { + tenancyId: options.tenancy.id, + permissionId: options.permissionId, + }, + select: { id: true }, + }); + for (const perm of teamPermissions) { + await recordExternalDbSyncDeletion(sourceOfTruthTx, { + tableName: "TeamMemberDirectPermission", + tenancyId: options.tenancy.id, + permissionDbId: perm.id, + }); + } await sourceOfTruthTx.teamMemberDirectPermission.deleteMany({ where: { tenancyId: options.tenancy.id, permissionId: options.permissionId, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/permissions.tsx` around lines 433 - 460, The team-scope branch currently calls sourceOfTruthTx.teamMemberDirectPermission.deleteMany(...) directly and thus skips recording tombstones; mirror the project-scope logic by first fetching matching teamMemberDirectPermission rows (use sourceOfTruthTx.teamMemberDirectPermission.findMany with where { tenancyId: options.tenancy.id, permissionId: options.permissionId } and select { id: true }), loop over each result and call recordExternalDbSyncDeletion(sourceOfTruthTx, { tableName: "TeamMemberDirectPermission", tenancyId: options.tenancy.id, permissionDbId: perm.id }), then call the existing teamMemberDirectPermission.deleteMany(...) to remove them.
330-348:⚠️ Potential issue | 🟠 MajorMark permission renames for resync.
These
updateManycalls changepermissionIdin place but leaveshouldUpdateSequenceIduntouched, so syncedteam_permissions/project_permissionsrows can keep the old permission id until some unrelated write touches them.Possible fix
await sourceOfTruthTx.teamMemberDirectPermission.updateMany({ where: { tenancyId: options.tenancy.id, permissionId: options.oldId, }, - data: { - permissionId: newId, - }, + data: withExternalDbSyncUpdate({ + permissionId: newId, + }), }); await sourceOfTruthTx.projectUserDirectPermission.updateMany({ where: { tenancyId: options.tenancy.id, permissionId: options.oldId, }, - data: { - permissionId: newId, - }, + data: withExternalDbSyncUpdate({ + permissionId: newId, + }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/permissions.tsx` around lines 330 - 348, The updateMany calls on sourceOfTruthTx.teamMemberDirectPermission.updateMany and sourceOfTruthTx.projectUserDirectPermission.updateMany only change permissionId and must also mark those rows for resync; update the data payload in both calls to set shouldUpdateSequenceId (e.g., to null or the sentinel value your sync uses) alongside permissionId: newId so the downstream sync will re-evaluate and propagate the renamed permission immediately.apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts (1)
236-283:⚠️ Potential issue | 🟠 MajorInclude
contact_channelsinmapping_status.Line 369 still computes
contactChannelStats, but this helper only emitsusersandemail_outboxes. As a result,sync_engine.mappingsandexternal_databases[].mapping_statuswill never report backlog forcontact_channels, even though the endpoint already exposessequencer.contact_channels.Also applies to: 368-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts` around lines 236 - 283, buildMappingInternalStats currently only sets mappings for "users" and "email_outboxes", so "contact_channels" never appears in mapping_status; add a new mapping entry for contact_channels using the existing contactChannelStats (and corresponding deletedRowsByTable lookup for ContactChannel similar to deletedProjectUserStats) and compute internal_min_sequence_id/internal_max_sequence_id/internal_pending_count with minBigIntString, maxBigIntString and addBigIntStrings just like you do for "users", then ensure mappingInternalStats.set("contact_channels", { mapping_id: "contact_channels", internal_min_sequence_id: ..., internal_max_sequence_id: ..., internal_pending_count: ... }) so contact_channels shows up in mappings and mappingStatuses.
🧹 Nitpick comments (1)
apps/backend/scripts/run-cron-jobs.ts (1)
33-33: Make the 1s loop test-only or configurable.This now drives both internal cron endpoints at a fixed 1Hz in every environment. That's a large steady-state traffic increase over the old jittered delay, and removing the jitter also makes replica bursts line up again. An env-controlled fast path would preserve the test-speed win without baking the hot polling cadence into normal deployments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/run-cron-jobs.ts` at line 33, The loop currently awaiting wait(1000) forces a 1s fixed poll in all environments; change it to use a configurable interval and restore non-deterministic/backoff behavior for production. Replace the hardcoded wait(1000) in run-cron-jobs.ts with a value derived from an env var (e.g., CRON_POLL_MS or a boolean TEST_FAST_CRON) so tests can opt into 1s polling while normal runs use a larger, jittered/default interval; or compute the delay when the loop starts (use wait(getCronDelayMs())) where getCronDelayMs() returns test-configured value if process.env.TEST_FAST_CRON is set, otherwise returns a randomized/backoff delay to avoid aligned bursts. Ensure references to wait(...) and the loop calling it are updated and documented in env usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/backend/prisma/migrations/20260316000000_add_team_team_member_sequence_columns/migration.sql`:
- Around line 5-12: This migration creates several indexes for Team
(Team_sequenceId_key, Team_tenancyId_sequenceId_idx,
Team_shouldUpdateSequenceId_idx) (and similarly for TeamMember at lines 18-25)
inside the Prisma transaction which will block writes on large tables; split
these index creations into one or more follow-up migrations that run OUTSIDE the
migration transaction and build the indexes CONCURRENTLY (use CREATE INDEX
CONCURRENTLY / CREATE UNIQUE INDEX CONCURRENTLY where appropriate), and add the
CONDITIONALLY_REPEAT_MIGRATION_SENTINEL pattern and any temporary marker columns
needed to perform the change safely on >1,000,000-row tables so the original
transactional migration no longer includes these plain index builds.
In
`@apps/backend/prisma/migrations/20260316000001_add_email_outbox_sequence_columns/migration.sql`:
- Around line 5-12: The three indexes on EmailOutbox
("EmailOutbox_sequenceId_key", "EmailOutbox_tenancyId_sequenceId_idx",
"EmailOutbox_shouldUpdateSequenceId_idx") must not be created inside this
transactional migration; instead remove these CREATE INDEX statements from this
migration and add separate non-transactional/concurrent migration files
following the repo's large-table pattern (use CREATE INDEX CONCURRENTLY and the
project’s concurrent/sentinel migration flow) so the index builds run outside a
transaction and as their own migration steps for the EmailOutbox table.
In
`@apps/backend/prisma/migrations/20260316000002_add_session_replay_sequence_columns/migration.sql`:
- Around line 5-12: The migration currently creates three indexes
("SessionReplay_sequenceId_key", "SessionReplay_tenancyId_sequenceId_idx",
"SessionReplay_shouldUpdateSequenceId_idx") inside the transactional migration
on table "SessionReplay"; remove these CREATE INDEX statements from this
migration and instead add one or more follow-up migrations that build the
indexes using CREATE INDEX CONCURRENTLY (or the DB-specific non-blocking
concurrent option), ensuring you follow the repository's safe-migration pattern
(use CONDITIONALLY_REPEAT_MIGRATION_SENTINEL and any temporary marking columns
needed) so the index builds run outside the transaction and can be retried
safely for large tables.
In
`@apps/backend/prisma/migrations/20260317000000_add_team_permission_invitation_sequence_columns/migration.sql`:
- Around line 5-22: The current migration creates heavy indexes inline
(TeamMemberDirectPermission_sequenceId_key,
TeamMemberDirectPermission_shouldUpdateSequenceId_idx,
TeamMemberDirectPermission_tenancyId_sequenceId_idx,
VerificationCode_sequenceId_key,
VerificationCode_shouldUpdateSequenceId_type_idx) inside a transaction which can
block large tables; instead keep only safe DDL in this migration (e.g., ALTER
TABLE "VerificationCode" ADD COLUMN "sequenceId" BIGINT and ADD COLUMN
"shouldUpdateSequenceId" BOOLEAN DEFAULT true) and move all index creation into
separate migration files that follow the large-table/sentinel pattern: create a
sentinel column if needed, run CREATE INDEX CONCURRENTLY ... for each index (use
the exact index names above), then run a small follow-up migration to set the
sentinel flags/visibility and finally drop the sentinel column; ensure the
concurrent index migrations are not wrapped in a transaction (use CREATE INDEX
CONCURRENTLY) and split them into distinct migration sql files.
In
`@apps/backend/prisma/migrations/20260317000001_add_project_permission_notification_preference_sequence_columns/migration.sql`:
- Around line 5-25: This migration adds two new columns ("sequenceId",
"shouldUpdateSequenceId") to UserNotificationPreference and then creates six
indexes (ProjectUserDirectPermission_sequenceId_key,
ProjectUserDirectPermission_shouldUpdateSequenceId_idx,
ProjectUserDirectPermission_tenancyId_sequenceId_idx,
UserNotificationPreference_sequenceId_key,
UserNotificationPreference_shouldUpdateSequenceId_idx,
UserNotificationPreference_tenancyId_sequenceId_idx) inside the same
transactional migration, which risks long-running locks on large tables; split
this up by keeping only the fast, transactional schema change (ALTER TABLE to
add the "sequenceId" and "shouldUpdateSequenceId" columns in the current
migration) and move all CREATE INDEX statements into one or more separate
non-transactional large-table migrations following the repo's
concurrent/sentinel pattern (use CREATE INDEX CONCURRENTLY in those migrations
and the repo’s sentinel workflow for safe rollouts) so the index builds run
outside the transaction for ProjectUserDirectPermission and
UserNotificationPreference.
In
`@apps/backend/prisma/migrations/20260318000000_add_sequence_id_to_refresh_tokens_and_oauth_accounts/migration.sql`:
- Around line 5-25: This migration must be split into safe, non-transactional
index-creation steps: remove the six CREATE INDEX / CREATE UNIQUE INDEX
statements from this transactional file (the lines creating
ProjectUserRefreshToken_sequenceId_key,
ProjectUserRefreshToken_shouldUpdateSequenceId_idx,
ProjectUserRefreshToken_tenancyId_sequenceId_idx,
ProjectUserOAuthAccount_sequenceId_key,
ProjectUserOAuthAccount_shouldUpdateSequenceId_idx,
ProjectUserOAuthAccount_tenancyId_sequenceId_idx) and keep only the ALTER TABLE
that adds sequenceId and shouldUpdateSequenceId; then add separate migration
files that (a) use the repo’s CONDITIONALLY_REPEAT_MIGRATION_SENTINEL pattern
and a temporary marking column if needed, (b) run outside the transaction or
explicitly create indexes CONCURRENTLY on ProjectUserRefreshToken and
ProjectUserOAuthAccount (creating the unique index on sequenceId and the two
non-unique indexes referencing shouldUpdateSequenceId and tenancyId), and (c)
ensure each index build is in its own migration to avoid Prisma transaction
timeouts.
In `@apps/backend/src/app/api/latest/auth/password/update/route.tsx`:
- Around line 82-88: Wrap the tombstone recording and the deleteMany into a
single serializable Prisma transaction so they commit or abort together: call
globalPrismaClient.$transaction([...], { isolationLevel: 'Serializable' }) and
inside it invoke recordExternalDbSyncRefreshTokenDeletionsForUser (passing
tenancyId, projectUserId and excludeRefreshToken) and then
globalPrismaClient.projectUserRefreshToken.deleteMany with the same predicate;
this ensures the recorded tombstones and the deleted set are aligned and
prevents phantom-insert windows.
In `@apps/backend/src/app/api/latest/auth/sessions/crud.tsx`:
- Around line 75-81: The tombstone insert (recordExternalDbSyncDeletion) is
currently committed before deleting the refresh token, risking inconsistent
external sync if the delete fails; wrap both operations in a single Prisma
transaction so they succeed or fail together. Modify the code to run
recordExternalDbSyncDeletion and
globalPrismaClient.projectUserRefreshToken.deleteMany inside a single
transaction (e.g., use globalPrismaClient.$transaction with an async callback or
pass both statements to $transaction) so the tombstone insert and delete are
atomic and reference the same tenancyId/refreshTokenId parameters.
In
`@apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts`:
- Around line 157-174: The UPDATE's subquery collapses changed teams to only
{projectId, branchId} (changed_teams), causing EVERY TEAM_INVITATION in that
tenancy to be marked; modify the subquery and WHERE join to preserve team-level
specificity by selecting "Team"."id" (or the appropriate team identifier used on
VerificationCode, e.g., "teamId") in changed_teams and add a matching condition
like AND "VerificationCode"."teamId" = changed_teams."id" (or the actual column
name) so only verification codes for the specific changed teams are toggled;
update the SELECT DISTINCT and the WHERE clause that references changed_teams
accordingly (symbols: "Team", "Tenancy", changed_teams, "VerificationCode").
- Around line 83-98: The cascade UPDATE currently re-queries all sequenced
ProjectUser rows per tenancy; change it to join only the exact (tenancyId,
projectUserId) pairs produced by the first CTE (the rows_to_update result) so
TeamMember rows are marked only for users updated in this batch. Locate the
globalPrismaClient.$executeRaw block that updates "TeamMember" and replace the
FROM subquery that selects from "ProjectUser" with a join to the first CTE (the
rows_to_update CTE name used earlier in this query) or propagate the updated
(tenancyId, projectUserId) pairs into a temp/table expression and join on those
pairs, keeping the existing filters (shouldUpdateSequenceId = FALSE, sequenceId
IS NOT NULL) but applying them only to the rows_to_update set.
In `@apps/backend/src/lib/external-db-sync.ts`:
- Around line 274-302: The permission tombstone INSERT currently filters only by
permission id, which can record a deletion for the wrong tenant; update the
WHERE clause in the tx.$executeRaw calls for the "TeamMemberDirectPermission"
branch (and the similar branch at lines 312-339) to include tenancy ID filtering
by adding AND "tenancyId" = ${target.tenancyId}::uuid, and ensure
target.tenancyId is asserted/validated (target.tenancyId /
target.permissionDbId) before running tx.$executeRaw so the lookup only affects
the intended tenant.
- Around line 454-485: The INSERT builds tombstones for VerificationCode but
scopes Tenancy only by projectId/branchId, so sibling tenancies can produce
extra rows; ensure the query restricts to the intended tenancy by validating and
using target.tenancyId and adding a Tenancy.id filter. Add a validation for
target.tenancyId (e.g., assertUuid or assertNonEmptyString depending on type)
and modify the tx.$executeRaw SQL (the JOIN/WHERE involving "Tenancy" and
"VerificationCode") to include AND "Tenancy"."id" = ${target.tenancyId} (or move
it into the JOIN condition) so the DeletedRow insert creates at most one row for
the specified tenancy; keep references to VerificationCode, Tenancy, DeletedRow,
and target.verificationCodeProjectId/verificationCodeBranchId/verificationCodeId
when making the change.
In `@apps/backend/src/route-handlers/verification-code-handler.tsx`:
- Around line 280-299: The external DB tombstone write
(recordExternalDbSyncDeletion) is done before and outside the verificationCode
delete, which can lead to inconsistent state if the delete fails; wrap the
conditional tombstone call and the globalPrismaClient.verificationCode.delete
together inside a single globalPrismaClient.$transaction so both operations
succeed or fail atomically (move the call to recordExternalDbSyncDeletion into
the transaction callback alongside the delete, using the same transaction
client), referencing recordExternalDbSyncDeletion,
globalPrismaClient.verificationCode.delete and globalPrismaClient.$transaction
to locate the changes.
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts`:
- Around line 699-741: Add a post-membership user-update case to the "TeamMember
CRUD sync (Postgres)" test: after creating the membership (the addMemberResponse
and waitForSyncedTeamMember(client, teamId, user.userId) steps), update the
ProjectUser/User record (e.g., change display_name and/or primary_email via
User.update or the API using niceBackendFetch), then wait for the sync (reuse
waitForSyncedTeamMember or add a new wait helper) and query
"team_member_profiles" to assert the row reflects the updated
display_name/primary_email; finally, proceed to the existing remove-member and
waitForSyncedTeamMemberDeletion checks so the test covers insert,
post-membership update, and deletion.
In `@packages/stack-shared/src/config/db-sync-mappings.ts`:
- Around line 303-320: The tombstone rows are emitting NULL into non-null
ClickHouse columns which causes the first deletion sync to fail; update the
affected table definitions so deleted-row placeholders can be written by either
making the problematic columns nullable (e.g., LowCardinality(Nullable(String))
or Nullable(String)) or giving them non-null defaults (e.g., DEFAULT '') as
appropriate. Specifically, modify the analytics_internal.contact_channels CREATE
TABLE to allow NULL/default for contact_channels.type and contact_channels.value
(and likewise adjust teams.display_name and connected_accounts.provider and
connected_accounts.provider_account_id in their respective CREATE TABLEs
referenced in the review) so tombstone rows no longer violate the schema while
preserving existing ordering/partitioning keys.
- Around line 1111-1132: The query currently scopes by "Tenancy" via
projectId/branchId which can include sibling tenancies; instead make the "Team"
join authoritative by changing the LEFT JOIN "Team" to an INNER JOIN and adding
a tenancy filter on the team (e.g. add AND "Team"."tenancyId" = $1::uuid to the
"Team" ON clause) so rows are only returned for teams that belong to the target
tenancy; apply the same change to the other query block that starts at the
second occurrence (around lines 1160-1179) to ensure both queries are scoped by
"Team"."tenancyId" rather than only by "Tenancy".
- Around line 819-826: The mapping for last_active_at_millis incorrectly
extracts the createdAt timestamp; update the CASE expression in db-sync-mappings
so that when "ProjectUser"."lastActiveAt" IS NOT NULL you EXTRACT(EPOCH FROM
"ProjectUser"."lastActiveAt") * 1000 instead of using "ProjectUser"."createdAt"
(keep the existing NULL branch and overall structure that builds the "user"
jsonb). This change targets the CASE producing "last_active_at_millis" in the
db-sync-mappings file.
- Line 667: The team_member_profiles sync currently lists ProjectUser in
sourceTables but always advances the cursor using
TeamMember.sequenceId/sequence_id, so updates to ProjectUser never move its
cursor and embedded user payloads go stale; update the team_member_profiles
fetch/advance logic (where you reference sourceTables, TeamMember, ProjectUser,
sequenceId/sequence_id) to advance the cursor based on the originating source:
when the row came from ProjectUser use ProjectUser.sequenceId/sequence_id, when
from TeamMember use TeamMember.sequenceId/sequence_id (or maintain separate
per-source cursors), and ensure the SELECT/WHERE and subsequent cursor-update
statements reference the correct sequence column so ProjectUser changes trigger
re-sync of the embedded user payload.
---
Outside diff comments:
In `@apps/backend/src/app/api/latest/auth/sessions/current/route.tsx`:
- Around line 37-48: Run the delete and the tombstone insertion inside a single
Prisma transaction so the tombstone cannot be committed without the source
delete: wrap globalPrismaClient.projectUserRefreshToken.deleteMany(...) and the
call to recordExternalDbSyncDeletion(...) in a single
globalPrismaClient.$transaction(...) and ensure you pass the delete result (the
deleted count) or an explicit expected-deleted-count to
recordExternalDbSyncDeletion so it can insert the 0-row mapping instead of
throwing; if needed, update recordExternalDbSyncDeletion to accept an
expectedCount/allowZero flag and avoid throwing when expectedCount === 0 while
still inserting the corresponding DeletedRow record for tenancy.id and
refreshTokenId.
In `@apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts`:
- Around line 236-283: buildMappingInternalStats currently only sets mappings
for "users" and "email_outboxes", so "contact_channels" never appears in
mapping_status; add a new mapping entry for contact_channels using the existing
contactChannelStats (and corresponding deletedRowsByTable lookup for
ContactChannel similar to deletedProjectUserStats) and compute
internal_min_sequence_id/internal_max_sequence_id/internal_pending_count with
minBigIntString, maxBigIntString and addBigIntStrings just like you do for
"users", then ensure mappingInternalStats.set("contact_channels", { mapping_id:
"contact_channels", internal_min_sequence_id: ..., internal_max_sequence_id:
..., internal_pending_count: ... }) so contact_channels shows up in mappings and
mappingStatuses.
In `@apps/backend/src/app/api/latest/users/crud.tsx`:
- Around line 1193-1203: Wrap the tombstone write and the actual deletion in a
single transaction so the external-db-sync deletions are only emitted if the DB
delete succeeds: move the calls to
recordExternalDbSyncRefreshTokenDeletionsForUser(...) and
globalPrismaClient.projectUserRefreshToken.deleteMany(...) into a single
globalPrismaClient.$transaction(...) and run them there (ensure you pass both
operations as transactional actions so they either both commit or both roll
back). Make sure to preserve the same parameters (auth.tenancy.id and
params.user_id) and return/await the transaction result as appropriate.
In `@apps/backend/src/lib/permissions.tsx`:
- Around line 433-460: The team-scope branch currently calls
sourceOfTruthTx.teamMemberDirectPermission.deleteMany(...) directly and thus
skips recording tombstones; mirror the project-scope logic by first fetching
matching teamMemberDirectPermission rows (use
sourceOfTruthTx.teamMemberDirectPermission.findMany with where { tenancyId:
options.tenancy.id, permissionId: options.permissionId } and select { id: true
}), loop over each result and call recordExternalDbSyncDeletion(sourceOfTruthTx,
{ tableName: "TeamMemberDirectPermission", tenancyId: options.tenancy.id,
permissionDbId: perm.id }), then call the existing
teamMemberDirectPermission.deleteMany(...) to remove them.
- Around line 330-348: The updateMany calls on
sourceOfTruthTx.teamMemberDirectPermission.updateMany and
sourceOfTruthTx.projectUserDirectPermission.updateMany only change permissionId
and must also mark those rows for resync; update the data payload in both calls
to set shouldUpdateSequenceId (e.g., to null or the sentinel value your sync
uses) alongside permissionId: newId so the downstream sync will re-evaluate and
propagate the renamed permission immediately.
---
Nitpick comments:
In `@apps/backend/scripts/run-cron-jobs.ts`:
- Line 33: The loop currently awaiting wait(1000) forces a 1s fixed poll in all
environments; change it to use a configurable interval and restore
non-deterministic/backoff behavior for production. Replace the hardcoded
wait(1000) in run-cron-jobs.ts with a value derived from an env var (e.g.,
CRON_POLL_MS or a boolean TEST_FAST_CRON) so tests can opt into 1s polling while
normal runs use a larger, jittered/default interval; or compute the delay when
the loop starts (use wait(getCronDelayMs())) where getCronDelayMs() returns
test-configured value if process.env.TEST_FAST_CRON is set, otherwise returns a
randomized/backoff delay to avoid aligned bursts. Ensure references to wait(...)
and the loop calling it are updated and documented in env usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3c429cf-f64b-4a0a-9826-3cec8b2c92f7
📒 Files selected for processing (29)
apps/backend/prisma/migrations/20260316000000_add_team_team_member_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260316000001_add_email_outbox_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260316000002_add_session_replay_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260317000000_add_team_permission_invitation_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260317000001_add_project_permission_notification_preference_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260318000000_add_sequence_id_to_refresh_tokens_and_oauth_accounts/migration.sqlapps/backend/prisma/schema.prismaapps/backend/scripts/clickhouse-migrations.tsapps/backend/scripts/run-cron-jobs.tsapps/backend/src/app/api/latest/auth/password/update/route.tsxapps/backend/src/app/api/latest/auth/sessions/crud.tsxapps/backend/src/app/api/latest/auth/sessions/current/route.tsxapps/backend/src/app/api/latest/emails/notification-preference/crud.tsxapps/backend/src/app/api/latest/emails/outbox/crud.tsxapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/status/route.tsapps/backend/src/app/api/latest/oauth-providers/crud.tsxapps/backend/src/app/api/latest/session-replays/batch/route.tsxapps/backend/src/app/api/latest/team-memberships/crud.tsxapps/backend/src/app/api/latest/teams/crud.tsxapps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/lib/email-queue-step.tsxapps/backend/src/lib/external-db-sync.tsapps/backend/src/lib/permissions.tsxapps/backend/src/route-handlers/verification-code-handler.tsxapps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.tspackages/stack-shared/src/config/db-sync-mappings.ts
N2D4
approved these changes
Apr 1, 2026
Contributor
N2D4
left a comment
N2D4
left a comment
There was a problem hiding this comment.
should be good, but please create the verify-data-integrity stuff in a separate PR
N2D4
reviewed
Apr 1, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts (1)
252-291:⚠️ Potential issue | 🟠 MajorPopulate
mappingStatusesfor the rest of the new sync mappings.This function still emits internal checkpoints for only
usersandemail_outboxes. Sincepackages/stack-shared/src/config/db-sync-mappings.tsnow also definescontact_channels,teams,team_member_profiles,team_permissions,team_invitations,project_permissions,notification_preferences,refresh_tokens, andconnected_accounts,/internal/external-db-sync/statuswill under-report backlog for most of the new sync surfaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts` around lines 252 - 291, buildMappingInternalStats currently only emits entries for "users" and "email_outboxes"; add entries for the missing mappings (contact_channels, teams, team_member_profiles, team_permissions, team_invitations, project_permissions, notification_preferences, refresh_tokens, connected_accounts) by following the same pattern used for "users" and "email_outboxes": for each mapping compute internal_min_sequence_id/internal_max_sequence_id using minBigIntString/maxBigIntString and internal_pending_count using addBigIntStrings (or fall back to the deletedRowsByTable find for that table_name when appropriate), then call mappingInternalStats.set("<mapping_name>", { mapping_id: "<mapping_name>", internal_min_sequence_id, internal_max_sequence_id, internal_pending_count }). If you need dedicated SequenceStats per mapping, extend the buildMappingInternalStats signature to accept them and use those variables (same pattern as projectUsersStats/emailOutboxStats).
♻️ Duplicate comments (2)
apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts (1)
699-741:⚠️ Potential issue | 🟡 MinorCover the post-membership user update here.
This only proves membership insert/delete.
team_member_profilesalso changes when the underlying user changes, so a display-name or profile-image update after membership creation can still regress without tripping this test. As per coding guidelines:**/apps/e2e/**/*.{ts,tsx}: ALWAYS add new E2E tests when you change the API or SDK interface. Generally, err on the side of creating too many tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts` around lines 699 - 741, The test currently only verifies team_member_profiles on membership insert/delete; add a step to update the underlying user after membership creation (e.g., update User via the same model used in the test: User.update or modify the user instance created with User.create) and then call waitForSyncedTeamMember(client, teamId, user.userId) again to assert that the profile fields changed in the external DB (check display_name and/or profile_image via client.query on "team_member_profiles"); keep the existing membership-add and delete assertions and reuse functions like niceBackendFetch, waitForSyncedTeamMember, and the client.query to validate the persisted user update sync.apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts (1)
140-157:⚠️ Potential issue | 🟠 MajorScope the invitation cascade to the updated teams.
changed_teamscurrently collapses to{projectId, branchId}, so backfilling one team flipsshouldUpdateSequenceIdfor everyTEAM_INVITATIONrow in that tenancy. That can create a lot of unnecessary re-sync churn on unrelated invites. Carry the updated(tenancyId, teamId)pairs forward and join on the invite’s team id instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts` around lines 140 - 157, The cascade currently groups changed_teams by projectId/branchId which causes any TEAM_INVITATION in the whole tenancy to be marked; modify the subquery used in the raw SQL (the block that builds changed_teams in route.ts) to SELECT DISTINCT tenancyId, teamId (or map teamTenants to include both tenancyId and teamId) and then change the outer WHERE to join VerificationCode on its teamId (or tenancyId AND teamId) instead of only projectId/branchId; update the Prisma.join usage so the raw query carries the (tenancyId, teamId) pairs and ensure the WHERE includes "VerificationCode"."teamId" = changed_teams."teamId" to scope updates to only invites for the changed teams.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/scripts/clickhouse-migrations.ts`:
- Around line 57-62: The backfill can run before the row-format mutation
finishes because Promise.all launches
TOKEN_REFRESH_EVENT_ROW_FORMAT_MUTATION_SQL and
BACKFILL_REFRESH_TOKEN_ID_COLUMN_SQL concurrently; fix by either making
BACKFILL_REFRESH_TOKEN_ID_COLUMN_SQL tolerant to both data.refresh_token_id and
data.refreshTokenId (use a COALESCE/ifEmpty/JSON extract that checks both keys)
or by serializing the operations: await client.command({ query:
TOKEN_REFRESH_EVENT_ROW_FORMAT_MUTATION_SQL }) before calling client.command({
query: BACKFILL_REFRESH_TOKEN_ID_COLUMN_SQL }) instead of running them inside
Promise.all. Ensure you update the Promise.all block that references
TOKEN_REFRESH_EVENT_ROW_FORMAT_MUTATION_SQL and
BACKFILL_REFRESH_TOKEN_ID_COLUMN_SQL accordingly.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/external-db-sync/page-client.tsx:
- Around line 548-549: The sequencer table is displaying rows using
status?.sequencer while loadStatus() is always called with scope='all', causing
a mix of global totals in the summary cards and tenancy-scoped rows in the
table; update the sequencer table to use the same scope source as the summary
cards by deriving the scope from the same variable/state/prop used when calling
loadStatus() (or change loadStatus to accept and use the current scope used by
the cards), and ensure the value passed into loadStatus() and the value used to
read status (status?.sequencer) are the identical scope so both cards and table
reflect the same tenancy scope (also apply same change to the other occurrences
around the second block that reads status?.sequencer).
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts`:
- Around line 1131-1174: Add a second team and a rename-after-invite scenario to
this TeamInvitation sync test: after creating the first team and sending its
invite (using niceBackendFetch and waitForSyncedTeamInvitation), create a second
team and send an invite for it, then rename the first team (PATCH/PUT the team
via the same API used elsewhere) and wait for the sequencer sync (reuse
waitForSyncedTeamInvitation) to assert the first invite's team_display_name
updated to the new name while the second invite's team_display_name remains
unchanged; also assert deletions still work using
waitForSyncedTeamInvitationDeletion and reference the existing helpers and
symbols like niceBackendFetch, waitForSyncedTeamInvitation,
waitForSyncedTeamInvitationDeletion, dbManager.getClient and the
"team_invitations" query to locate where to add these steps.
---
Outside diff comments:
In `@apps/backend/src/app/api/latest/internal/external-db-sync/status/route.ts`:
- Around line 252-291: buildMappingInternalStats currently only emits entries
for "users" and "email_outboxes"; add entries for the missing mappings
(contact_channels, teams, team_member_profiles, team_permissions,
team_invitations, project_permissions, notification_preferences, refresh_tokens,
connected_accounts) by following the same pattern used for "users" and
"email_outboxes": for each mapping compute
internal_min_sequence_id/internal_max_sequence_id using
minBigIntString/maxBigIntString and internal_pending_count using
addBigIntStrings (or fall back to the deletedRowsByTable find for that
table_name when appropriate), then call
mappingInternalStats.set("<mapping_name>", { mapping_id: "<mapping_name>",
internal_min_sequence_id, internal_max_sequence_id, internal_pending_count }).
If you need dedicated SequenceStats per mapping, extend the
buildMappingInternalStats signature to accept them and use those variables (same
pattern as projectUsersStats/emailOutboxStats).
---
Duplicate comments:
In
`@apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts`:
- Around line 140-157: The cascade currently groups changed_teams by
projectId/branchId which causes any TEAM_INVITATION in the whole tenancy to be
marked; modify the subquery used in the raw SQL (the block that builds
changed_teams in route.ts) to SELECT DISTINCT tenancyId, teamId (or map
teamTenants to include both tenancyId and teamId) and then change the outer
WHERE to join VerificationCode on its teamId (or tenancyId AND teamId) instead
of only projectId/branchId; update the Prisma.join usage so the raw query
carries the (tenancyId, teamId) pairs and ensure the WHERE includes
"VerificationCode"."teamId" = changed_teams."teamId" to scope updates to only
invites for the changed teams.
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts`:
- Around line 699-741: The test currently only verifies team_member_profiles on
membership insert/delete; add a step to update the underlying user after
membership creation (e.g., update User via the same model used in the test:
User.update or modify the user instance created with User.create) and then call
waitForSyncedTeamMember(client, teamId, user.userId) again to assert that the
profile fields changed in the external DB (check display_name and/or
profile_image via client.query on "team_member_profiles"); keep the existing
membership-add and delete assertions and reuse functions like niceBackendFetch,
waitForSyncedTeamMember, and the client.query to validate the persisted user
update sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c659f109-0950-499e-babb-cccd2e4b297f
📒 Files selected for processing (21)
apps/backend/prisma/migrations/20260316000000_add_team_team_member_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260316000001_add_email_outbox_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260316000002_add_session_replay_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260317000000_add_team_permission_invitation_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260317000001_add_project_permission_notification_preference_sequence_columns/migration.sqlapps/backend/prisma/migrations/20260318000000_add_sequence_id_to_refresh_tokens_and_oauth_accounts/migration.sqlapps/backend/prisma/migrations/20260318000001_add_sequence_indexes_concurrently/migration.sqlapps/backend/prisma/schema.prismaapps/backend/scripts/clickhouse-migrations.tsapps/backend/src/app/api/latest/emails/unsubscribe-link/route.tsxapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/app/api/latest/internal/external-db-sync/status/route.tsapps/backend/src/app/api/latest/oauth-providers/crud.tsxapps/backend/src/app/api/latest/team-member-profiles/crud.tsxapps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/lib/email-queue-step.tsxapps/backend/src/lib/external-db-sync.tsapps/backend/src/lib/permissions.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/external-db-sync/page-client.tsxapps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts
✅ Files skipped from review due to trivial changes (4)
- apps/backend/prisma/migrations/20260316000000_add_team_team_member_sequence_columns/migration.sql
- apps/backend/prisma/migrations/20260318000001_add_sequence_indexes_concurrently/migration.sql
- apps/backend/src/lib/external-db-sync.ts
- apps/backend/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/backend/prisma/migrations/20260317000001_add_project_permission_notification_preference_sequence_columns/migration.sql
- apps/backend/prisma/migrations/20260316000002_add_session_replay_sequence_columns/migration.sql
- apps/backend/src/app/api/latest/users/crud.tsx
- apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts
- apps/backend/prisma/migrations/20260316000001_add_email_outbox_sequence_columns/migration.sql
- apps/backend/prisma/migrations/20260317000000_add_team_permission_invitation_sequence_columns/migration.sql
- apps/backend/prisma/migrations/20260318000000_add_sequence_id_to_refresh_tokens_and_oauth_accounts/migration.sql
- apps/backend/src/lib/permissions.tsx
mantrakp04
requested changes
Apr 7, 2026
BilalG1
added a commit
that referenced
this pull request
Apr 8, 2026
- Narrow team invitation cascade to specific changed teamId instead of entire tenancy - Populate status dashboard stats for all 11 sync mappings, not just users and email_outboxes - Wrap all OAuth provider updates with withExternalDbSyncUpdate so allowSignIn and allowConnectedAccounts changes trigger sync - Remove session_replays from sequencer and column type map since no mapping exists yet - Add DeletedRow union to email_outboxes fetch queries for future safety
mantrakp04
approved these changes
Apr 8, 2026
Collaborator
Author
|
Closing in favor of #1304 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores