e2e: retry forced external DB sync until idle#1150
Closed
BilalG1 wants to merge 46 commits into
Closed
Conversation
This reverts commit 937db90.
Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
…er/route.ts Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
…er/route.ts Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a complete external database synchronization system enabling Stack to mirror user data from internal PostgreSQL databases to external customer-managed databases. It includes database migrations with sync tracking, new backend API routes for data sequencing and polling, environment configuration for sync control, workflow updates for continuous sync operations, and comprehensive end-to-end test coverage. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Webhook
participant API as Backend API
participant Queue as Sync Queue<br/>(OutgoingRequest)
participant Sequencer as Sequencer Route
participant Poller as Poller Route
participant QStash as QStash/Direct Sync
participant Internal as Internal DB
participant External as External DB
participant SyncEngine as Sync Engine
Client->>API: Update config with<br/>externalDatabases
API->>Queue: enqueueExternalDbSync(tenancyId)
Queue->>Internal: INSERT OutgoingRequest
loop Every 60 seconds (Cron)
Sequencer->>Internal: Backfill sequence IDs
Sequencer->>Internal: Mark rows for sync<br/>(shouldUpdateSequenceId)
Poller->>Internal: Claim pending<br/>OutgoingRequest rows
alt Direct Sync Enabled
Poller->>API: POST /sync-engine<br/>(direct HTTP)
else QStash Enabled
Poller->>QStash: Publish request
QStash->>SyncEngine: Webhook callback
end
end
SyncEngine->>Internal: Fetch tenancy config
SyncEngine->>External: syncExternalDatabases()
External->>Internal: Fetch rows with<br/>sequence > lastSynced
Internal-->>External: Return denormalized rows
External->>External: Upsert to target table
External->>External: Update _stack_sync_metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile OverviewGreptile SummaryThis PR introduces a complete external database synchronization feature and implements retry logic to fix CI flakes. The retry mechanism runs multiple sequencer/poller passes when forcing sync in tests, ensuring all pending operations complete before assertions. Key Changes:
Architecture:
The retry logic ensures deterministic test behavior by repeatedly running sequencer/poller until the system reaches an idle state with no pending work. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant Utils as external-db-sync-utils
participant Sequencer as Sequencer Endpoint
participant Poller as Poller Endpoint
participant Queue as OutgoingRequest Queue
participant SyncEngine as Sync Engine
participant InternalDB as Internal Postgres
participant ExternalDB as External Postgres
Note over Test,ExternalDB: CI Priming Phase (Before Tests)
Test->>Utils: Prime sync before tests
loop 3 times
Utils->>Sequencer: GET with stopWhenIdle=true
Sequencer->>InternalDB: Backfill sequence IDs
Sequencer->>Queue: Enqueue sync requests
Sequencer-->>Utils: {iterations: N}
Utils->>Poller: GET with stopWhenIdle=true
Poller->>Queue: Claim pending requests
Poller->>SyncEngine: POST sync request
SyncEngine->>InternalDB: Fetch changed rows
SyncEngine->>ExternalDB: Upsert data
SyncEngine-->>Poller: Success
Poller->>Queue: Delete request
Poller-->>Utils: {requests_processed: M}
Note over Utils: If iterations=0 AND requests=0, break
end
Note over Test,ExternalDB: Test Execution Phase
Test->>InternalDB: Create/Update user
InternalDB->>InternalDB: Mark shouldUpdateSequenceId=true
Test->>Utils: waitForSyncedData(email)
loop Until condition met or timeout
Utils->>Utils: maybeForceExternalDbSync()
Note over Utils: If >2s since last sync
Utils->>Sequencer: Retry loop (3 attempts)
Sequencer->>InternalDB: Assign sequenceId
Sequencer->>Queue: Enqueue if needed
Utils->>Poller: Retry loop (3 attempts)
Poller->>Queue: Process requests
Poller->>SyncEngine: Trigger sync
SyncEngine->>ExternalDB: Write changes
Utils->>ExternalDB: Query for data
alt Data found with correct value
ExternalDB-->>Utils: Row exists
Utils-->>Test: Condition satisfied
else Data not ready
Utils->>Utils: Wait 500ms, retry
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Comment on lines
+35
to
+36
| console.warn(`[sync-engine] Tenancy ${tenancyId} in queue but not found.`); | ||
| throw new StatusError(404, `Tenancy ${tenancyId} not found.`); |
There was a problem hiding this comment.
Suggested change
| console.warn(`[sync-engine] Tenancy ${tenancyId} in queue but not found.`); | |
| throw new StatusError(404, `Tenancy ${tenancyId} not found.`); | |
| console.warn(`[sync-engine] Tenancy ${tenancyId} in queue but not found.`); | |
| throw new StatusError(404, `Tenancy ${tenancyId} not found.`); |
Incorrect indentation in if block: console.warn and throw statements not properly indented inside if condition
|
|
||
| // Queues a sync request for a specific tenant if one isn't already pending. | ||
| export async function enqueueExternalDbSync(tenancyId: string): Promise<void> { | ||
| assertUuid(tenancyId, "tenancyId"); |
There was a problem hiding this comment.
Race condition in enqueueExternalDbSync allows duplicate pending sync requests for the same tenant due to non-atomic INSERT...WHERE NOT EXISTS pattern in READ COMMITTED isolation level
| const sequencerRes = await niceFetch(new URL('/api/latest/internal/external-db-sync/sequencer', STACK_BACKEND_BASE_URL), { | ||
| query: { | ||
| maxDurationMs: String(FORCE_SYNC_MAX_DURATION_MS), | ||
| stopWhenIdle: "true", |
There was a problem hiding this comment.
forceExternalDbSync function breaks retry loop on first idle pass without confirming cascading syncs are complete
| export const POSTGRES_USER = process.env.EXTERNAL_DB_TEST_USER || 'postgres'; | ||
| export const POSTGRES_PASSWORD = process.env.EXTERNAL_DB_TEST_PASSWORD || 'PASSWORD-PLACEHOLDER--uqfEC1hmmv'; | ||
| export const TEST_TIMEOUT = 120000; | ||
| export const HIGH_VOLUME_TIMEOUT = 600000; // 10 minutes for 1500+ users |
There was a problem hiding this comment.
Missing STACK_FORCE_EXTERNAL_DB_SYNC environment variable in .env.development causes tests to rely on slow automatic cron job instead of proactive forced syncs
![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: 5
🤖 Fix all issues with AI agents
In @.github/workflows/e2e-custom-base-port-api-tests.yaml:
- Around line 143-151: The CI step currently starts the background job using the
development cron script name "run-cron-jobs", which reads .env.development;
update the GitHub Action step that uses JarvusInnovations/background-action and
the run command to invoke the test variant (e.g., replace the current "pnpm -C
apps/backend run run-cron-jobs --log-order=stream &" with the test script name
such as "pnpm -C apps/backend run run-cron-jobs:test --log-order=stream &" so
the job uses the .env.test* environment for e2e tests; keep the rest of the step
(wait-on, tail, wait-for, log-output-if) unchanged.
In @.github/workflows/e2e-source-of-truth-api-tests.yaml:
- Around line 150-158: The CI cron job step currently invokes the script
"run-cron-jobs" which uses with-env:dev and loads .env.development; change the
invocation to "run-cron-jobs:test" so the job runs with the test environment
(.env.test). Locate the workflow step that uses
JarvusInnovations/background-action and replace the run command referencing
run-cron-jobs with run-cron-jobs:test (ensure the rest of the action inputs like
wait-on, tail, and wait-for remain unchanged).
In `@apps/e2e/tests/backend/endpoints/api/v1/auth/sessions/index.test.ts`:
- Around line 103-104: The timeout assertion currently in the finally block
(throwing when timeSinceBeginDate > 6_000) should be moved out of the finally so
it cannot mask exceptions from the try block; instead compute elapsed using
performance.now() (capture start with const begin = performance.now() and
compute timeSinceBeginDate = performance.now() - begin) and perform the timeout
check/assert after the try/catch completes (not inside finally), replacing the
manual throw with a proper test assertion or error that runs only when no other
test error occurred; update references to timeSinceBeginDate and any beginDate
variables accordingly.
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts`:
- Around line 175-199: The loop currently trusts sequencerRes and pollerRes
bodies without verifying HTTP status, so non‑2xx responses (401/500) will be
treated as idle; update the logic after the niceFetch calls to validate
sequencerRes.status and pollerRes.status (or an isOk/is2xx helper) and throw an
error if either response is not 2xx before reading
iterations/requests_processed, referencing the variables sequencerRes and
pollerRes used in this block and keeping the existing break behavior only when
both responses are successful and their counts are zero.
- Around line 47-129: Change all catch parameters from any to unknown (e.g., in
TestDbManager.cleanup and other try/catch blocks) and replace silent swallows
with guarded handling: add a type-guard function (e.g., isPostgresError(err:
unknown): err is { code?: string; message?: string }) that checks err is
object/non-null and has message/code properties, then log meaningful details
(console.warn/processLogger.warn) when the guard passes and otherwise rethrow or
log the unknown value; also add a proper type for the externalDatabases
parameter in createProjectWithExternalDb (replace externalDatabases: any with a
defined interface/type describing host, port, user, password, dbName, ssl, etc.)
and apply the same unknown + guard pattern to all other catch blocks mentioned
so no errors are silently ignored.
🧹 Nitpick comments (7)
apps/backend/.env.development (1)
35-40: Email configuration ordering is semantically logical (static analysis nitpick).The static analysis tool suggests alphabetical ordering (PASSWORD before PORT), but the current order (HOST → PORT → SECURE → USERNAME → PASSWORD → SENDER) follows a logical configuration hierarchy. This semantic grouping is more intuitive than alphabetical sorting, so the warning can be safely ignored.
apps/e2e/.env.development (1)
14-14: Consider prefixingCRON_SECRETwithSTACK_.Per project conventions, environment variables should be prefixed with
STACK_(orNEXT_PUBLIC_STACK_if public) to ensure Turborepo picks up changes and improve readability. Consider renaming toSTACK_CRON_SECRET.Based on learnings: "Environment variables should be prefixed with
STACK_orNEXT_PUBLIC_STACK_(if public) to ensure Turborepo picks up changes and improve readability"apps/backend/src/app/api/latest/internal/external-db-sync/sync-engine/route.tsx (1)
34-37: Inconsistent indentation inside theifblock.Lines 35-36 are not indented relative to the surrounding
ifblock, which reduces readability.🔧 Proposed fix for indentation
const tenancy = await getTenancy(tenancyId); if (!tenancy) { -console.warn(`[sync-engine] Tenancy ${tenancyId} in queue but not found.`); -throw new StatusError(404, `Tenancy ${tenancyId} not found.`); + console.warn(`[sync-engine] Tenancy ${tenancyId} in queue but not found.`); + throw new StatusError(404, `Tenancy ${tenancyId} not found.`); }apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-advanced.test.ts (1)
955-955: Use.slice()instead of deprecated.substr().
String.prototype.substr()is deprecated. Use.slice(2, 11)for equivalent behavior.🔧 Proposed fix
- 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/app/api/latest/internal/external-db-sync/sequencer/route.ts (1)
16-39: Code duplication withpoller/route.ts.
parseMaxDurationMs,parseStopWhenIdle,UUID_REGEX, andassertUuidare duplicated frompoller/route.ts. Consider extracting these to a shared utility module (e.g.,@/lib/external-db-sync-utils.ts) to reduce duplication and ensure consistent behavior.apps/backend/src/lib/external-db-sync.ts (2)
66-79: Consider adding type annotation comment fornewRows: any[].The
any[]type is necessary because row shapes vary by mapping and come from raw SQL. A brief comment explaining this would help future maintainers understand why strict typing isn't feasible here.📝 Suggested documentation
async function pushRowsToExternalDb( externalClient: Client, tableName: string, + // Rows from internalDbFetchQuery have dynamic shapes per mapping; strict typing isn't feasible. newRows: any[], upsertQuery: string, expectedTenancyId: string, mappingId: string, ) {
192-202: BigInt to Number conversion may lose precision for extremely large sequence IDs.The conversion on line 196 (
Number(seq)) could lose precision ifsequence_idexceedsNumber.MAX_SAFE_INTEGER(~9 quadrillion). While unlikely in practice, consider usingBigIntthroughout or adding a safeguard.🛡️ Optional safeguard
const seq = row.sequence_id; if (seq != null) { const seqNum = typeof seq === 'bigint' ? Number(seq) : Number(seq); + if (!Number.isSafeInteger(seqNum)) { + throw new StackAssertionError(`sequence_id ${seq} exceeds safe integer range for mapping ${mappingId}.`); + } if (seqNum > maxSeqInBatch) { maxSeqInBatch = seqNum; } }
Comment on lines
+143
to
+151
| - name: Start run-cron-jobs in background | ||
| uses: JarvusInnovations/background-action@v1.0.7 | ||
| with: | ||
| run: pnpm -C apps/backend run run-cron-jobs --log-order=stream & | ||
| wait-on: | | ||
| http://localhost:6702 | ||
| tail: true | ||
| wait-for: 30s | ||
| log-output-if: true |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -path "*/apps/backend/*" | head -5Repository: stack-auth/stack-auth
Length of output: 92
🏁 Script executed:
fd "package.json" apps/backendRepository: stack-auth/stack-auth
Length of output: 90
🏁 Script executed:
rg "run-cron-jobs" --type json | head -20Repository: stack-auth/stack-auth
Length of output: 586
🏁 Script executed:
sed -n '130,160p' .github/workflows/e2e-custom-base-port-api-tests.yamlRepository: stack-auth/stack-auth
Length of output: 1022
🏁 Script executed:
rg "e2e.*test" --type yaml .github/workflows/ -lRepository: stack-auth/stack-auth
Length of output: 268
🏁 Script executed:
rg "run-email-queue|run-cron-jobs" apps/backend/package.jsonRepository: stack-auth/stack-auth
Length of output: 586
🏁 Script executed:
grep -A 5 "run-email-queue\|run-cron-jobs" .github/workflows/e2e-api-tests.yaml | head -30Repository: stack-auth/stack-auth
Length of output: 721
🏁 Script executed:
rg "run-email-queue" apps/backend/package.jsonRepository: stack-auth/stack-auth
Length of output: 430
🏁 Script executed:
cat apps/backend/package.json | jq '.scripts | keys[] | select(contains("email-queue"))'Repository: stack-auth/stack-auth
Length of output: 82
Use the test env for cron jobs in CI.
run-cron-jobs uses with-env:dev, which reads .env.development. Since this e2e test workflow sources .env.test.local, switch to the test script.
Suggested change
- run: pnpm -C apps/backend run run-cron-jobs --log-order=stream &
+ run: pnpm -C apps/backend run run-cron-jobs:test --log-order=stream &📝 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.
Suggested change
| - name: Start run-cron-jobs in background | |
| uses: JarvusInnovations/background-action@v1.0.7 | |
| with: | |
| run: pnpm -C apps/backend run run-cron-jobs --log-order=stream & | |
| wait-on: | | |
| http://localhost:6702 | |
| tail: true | |
| wait-for: 30s | |
| log-output-if: true | |
| - name: Start run-cron-jobs in background | |
| uses: JarvusInnovations/background-action@v1.0.7 | |
| with: | |
| run: pnpm -C apps/backend run run-cron-jobs:test --log-order=stream & | |
| wait-on: | | |
| http://localhost:6702 | |
| tail: true | |
| wait-for: 30s | |
| log-output-if: true |
🤖 Prompt for AI Agents
In @.github/workflows/e2e-custom-base-port-api-tests.yaml around lines 143 -
151, The CI step currently starts the background job using the development cron
script name "run-cron-jobs", which reads .env.development; update the GitHub
Action step that uses JarvusInnovations/background-action and the run command to
invoke the test variant (e.g., replace the current "pnpm -C apps/backend run
run-cron-jobs --log-order=stream &" with the test script name such as "pnpm -C
apps/backend run run-cron-jobs:test --log-order=stream &" so the job uses the
.env.test* environment for e2e tests; keep the rest of the step (wait-on, tail,
wait-for, log-output-if) unchanged.
Comment on lines
+150
to
+158
| - name: Start run-cron-jobs in background | ||
| uses: JarvusInnovations/background-action@v1.0.7 | ||
| with: | ||
| run: pnpm -C apps/backend run run-cron-jobs --log-order=stream & | ||
| wait-on: | | ||
| http://localhost:8102 | ||
| tail: true | ||
| wait-for: 30s | ||
| log-output-if: true |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "package.json" -path "*/apps/backend/*" | head -5Repository: stack-auth/stack-auth
Length of output: 92
🏁 Script executed:
cat ./apps/backend/package.json | grep -A 50 '"scripts"'Repository: stack-auth/stack-auth
Length of output: 4278
🏁 Script executed:
cat -n ./.github/workflows/e2e-source-of-truth-api-tests.yaml | sed -n '145,165p'Repository: stack-auth/stack-auth
Length of output: 799
Use the test environment for cron jobs in CI.
The run-cron-jobs script uses with-env:dev, which reads .env.development. In this e2e test workflow, switch to run-cron-jobs:test to apply test environment settings (.env.test).
🛠️ Suggested change
- run: pnpm -C apps/backend run run-cron-jobs --log-order=stream &
+ run: pnpm -C apps/backend run run-cron-jobs:test --log-order=stream &🤖 Prompt for AI Agents
In @.github/workflows/e2e-source-of-truth-api-tests.yaml around lines 150 - 158,
The CI cron job step currently invokes the script "run-cron-jobs" which uses
with-env:dev and loads .env.development; change the invocation to
"run-cron-jobs:test" so the job runs with the test environment (.env.test).
Locate the workflow step that uses JarvusInnovations/background-action and
replace the run command referencing run-cron-jobs with run-cron-jobs:test
(ensure the rest of the action inputs like wait-on, tail, and wait-for remain
unchanged).
Comment on lines
+103
to
+104
| if (timeSinceBeginDate > 6_000) { | ||
| throw new StackAssertionError(`Timeout error: Requests were too slow (${timeSinceBeginDate}ms > 6000ms); try again or try to understand why they were slow.`); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JavaScript finally block throw error override try catch does it mask
💡 Result:
Yes. In JavaScript, a finally block can mask/override what happened in try/catch.
- If
finallycompletes normally (noreturn/throw/break/continue), then the original completion fromtry/catchis preserved (return value stays, or the original exception keeps propagating). Per the spec: ifF.type(thefinallyresult) isnormal, the overall result is the earlier one; otherwise the overall result becomesF. [1] - If
finallyexecutes a control-flow statement likethroworreturn, it wins and replaces (“masks”) any prior return/throw fromtryorcatch. MDN explicitly notes that control flow infinally“mask[s] any completion value” fromtry/catch. [2]
Example (exception is masked by finally):
function f() {
try {
throw new Error("from try");
} catch (e) {
throw new Error("from catch");
} finally {
throw new Error("from finally"); // this is what the caller sees
}
}
f(); // throws "from finally"Rule of thumb: avoid return/throw in finally unless you intentionally want to override earlier returns/errors. [2]
Sources: ECMAScript spec (ES5.1) try-statement completion rules [1]; MDN try...catch...finally (“mask” behavior) [2].
🏁 Script executed:
# Read the actual file to understand the context better
cat -n apps/e2e/tests/backend/endpoints/api/v1/auth/sessions/index.test.ts | head -150 | tail -60Repository: stack-auth/stack-auth
Length of output: 2692
Move timeout assertion out of finally block and use performance.now() for timing.
Throwing from finally overrides any error from the try block, masking the actual test failure and making flaky runs harder to debug. Additionally, per coding guidelines, use performance.now() instead of Date.now() for measuring elapsed time.
Suggested refactor
- const beginDate = new Date();
+ const beginTime = performance.now();
const res2 = await niceBackendFetch("/api/v1/auth/sessions", {
accessType: "server",
method: "POST",
body: {
user_id: res.userId,
expires_in_millis: 5_000,
},
});
expect(res2).toMatchInlineSnapshot(`
NiceResponse {
"status": 200,
"body": {
"access_token": <stripped field 'access_token'>,
"refresh_token": <stripped field 'refresh_token'>,
},
"headers": Headers { <some fields may have been hidden> },
}
`);
const waitPromise = wait(5_001);
- try {
- const refreshSessionResponse1 = await niceBackendFetch("/api/v1/auth/sessions/current/refresh", {
- method: "POST",
- accessType: "client",
- headers: {
- "x-stack-refresh-token": res2.body.refresh_token
- },
- });
- expect(refreshSessionResponse1).toMatchInlineSnapshot(`
- NiceResponse {
- "status": 200,
- "body": { "access_token": <stripped field 'access_token'> },
- "headers": Headers { <some fields may have been hidden> },
- }
- `);
- backendContext.set({ userAuth: { accessToken: refreshSessionResponse1.body.access_token, refreshToken: res2.body.refresh_token } });
- await Auth.expectToBeSignedIn();
- } finally {
- const timeSinceBeginDate = new Date().getTime() - beginDate.getTime();
- if (timeSinceBeginDate > 6_000) {
- throw new StackAssertionError(`Timeout error: Requests were too slow (${timeSinceBeginDate}ms > 6000ms); try again or try to understand why they were slow.`);
- }
- }
+ const refreshSessionResponse1 = await niceBackendFetch("/api/v1/auth/sessions/current/refresh", {
+ method: "POST",
+ accessType: "client",
+ headers: {
+ "x-stack-refresh-token": res2.body.refresh_token
+ },
+ });
+ expect(refreshSessionResponse1).toMatchInlineSnapshot(`
+ NiceResponse {
+ "status": 200,
+ "body": { "access_token": <stripped field 'access_token'> },
+ "headers": Headers { <some fields may have been hidden> },
+ }
+ `);
+ backendContext.set({ userAuth: { accessToken: refreshSessionResponse1.body.access_token, refreshToken: res2.body.refresh_token } });
+ await Auth.expectToBeSignedIn();
+ const timeSinceBeginDate = performance.now() - beginTime;
+ if (timeSinceBeginDate > 6_000) {
+ throw new StackAssertionError(`Timeout error: Requests were too slow (${timeSinceBeginDate}ms > 6000ms); try again or try to understand why they were slow.`);
+ }📝 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.
Suggested change
| if (timeSinceBeginDate > 6_000) { | |
| throw new StackAssertionError(`Timeout error: Requests were too slow (${timeSinceBeginDate}ms > 6000ms); try again or try to understand why they were slow.`); | |
| const beginTime = performance.now(); | |
| const res2 = await niceBackendFetch("/api/v1/auth/sessions", { | |
| accessType: "server", | |
| method: "POST", | |
| body: { | |
| user_id: res.userId, | |
| expires_in_millis: 5_000, | |
| }, | |
| }); | |
| expect(res2).toMatchInlineSnapshot(` | |
| NiceResponse { | |
| "status": 200, | |
| "body": { | |
| "access_token": <stripped field 'access_token'>, | |
| "refresh_token": <stripped field 'refresh_token'>, | |
| }, | |
| "headers": Headers { <some fields may have been hidden> }, | |
| } | |
| `); | |
| const waitPromise = wait(5_001); | |
| const refreshSessionResponse1 = await niceBackendFetch("/api/v1/auth/sessions/current/refresh", { | |
| method: "POST", | |
| accessType: "client", | |
| headers: { | |
| "x-stack-refresh-token": res2.body.refresh_token | |
| }, | |
| }); | |
| expect(refreshSessionResponse1).toMatchInlineSnapshot(` | |
| NiceResponse { | |
| "status": 200, | |
| "body": { "access_token": <stripped field 'access_token'> }, | |
| "headers": Headers { <some fields may have been hidden> }, | |
| } | |
| `); | |
| backendContext.set({ userAuth: { accessToken: refreshSessionResponse1.body.access_token, refreshToken: res2.body.refresh_token } }); | |
| await Auth.expectToBeSignedIn(); | |
| const timeSinceBeginDate = performance.now() - beginTime; | |
| if (timeSinceBeginDate > 6_000) { | |
| throw new StackAssertionError(`Timeout error: Requests were too slow (${timeSinceBeginDate}ms > 6000ms); try again or try to understand why they were slow.`); | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 104-104: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🤖 Prompt for AI Agents
In `@apps/e2e/tests/backend/endpoints/api/v1/auth/sessions/index.test.ts` around
lines 103 - 104, The timeout assertion currently in the finally block (throwing
when timeSinceBeginDate > 6_000) should be moved out of the finally so it cannot
mask exceptions from the try block; instead compute elapsed using
performance.now() (capture start with const begin = performance.now() and
compute timeSinceBeginDate = performance.now() - begin) and perform the timeout
check/assert after the try/catch completes (not inside finally), replacing the
manual throw with a proper test assertion or error that runs only when no other
test error occurred; update references to timeSinceBeginDate and any beginDate
variables accordingly.
Comment on lines
+47
to
+129
| export class TestDbManager { | ||
| private setupClient: Client | null = null; | ||
| private databases: Map<string, Client> = new Map(); | ||
| private databaseNames: Set<string> = new Set(); | ||
|
|
||
| async init() { | ||
| this.setupClient = new Client({ | ||
| connectionString: `postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@${POSTGRES_HOST}/postgres`, | ||
| ...CLIENT_CONFIG, | ||
| }); | ||
| await this.setupClient.connect(); | ||
| } | ||
|
|
||
| async createDatabase(dbName: string): Promise<string> { | ||
| if (!this.setupClient) throw new Error('TestDbManager not initialized'); | ||
|
|
||
| const uniqueDbName = `${dbName}_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; | ||
| await this.setupClient.query(`CREATE DATABASE "${uniqueDbName}"`); | ||
| const connectionString = `postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@${POSTGRES_HOST}/${uniqueDbName}`; | ||
| const client = new Client({ | ||
| connectionString, | ||
| ...CLIENT_CONFIG, | ||
| }); | ||
| await client.connect(); | ||
|
|
||
| this.databases.set(dbName, client); | ||
| this.databaseNames.add(uniqueDbName); | ||
| return connectionString; | ||
| } | ||
|
|
||
| getClient(dbName: string): Client { | ||
| const client = this.databases.get(dbName); | ||
| if (!client) throw new Error(`Database ${dbName} not found`); | ||
| return client; | ||
| } | ||
|
|
||
| async cleanup() { | ||
| // First, clean up all project configs to stop the sync cron from trying to connect | ||
| await cleanupAllProjectConfigs(); | ||
|
|
||
| // Close all tracked database clients | ||
| const closePromises = Array.from(this.databases.values()).map(async (client) => { | ||
| try { | ||
| await Promise.race([ | ||
| client.end(), | ||
| new Promise((_, reject) => setTimeout(() => reject(new Error('Client close timeout')), 5000)), | ||
| ]); | ||
| } catch (err) { | ||
| // Ignore errors when closing clients - they may already be closed or timed out | ||
| } | ||
| }); | ||
| await Promise.all(closePromises); | ||
| this.databases.clear(); | ||
|
|
||
| if (this.setupClient) { | ||
| // Terminate all connections and drop databases | ||
| for (const dbName of this.databaseNames) { | ||
| try { | ||
| // Forcefully terminate ALL connections to this database | ||
| await this.setupClient.query(` | ||
| SELECT pg_terminate_backend(pg_stat_activity.pid) | ||
| FROM pg_stat_activity | ||
| WHERE pg_stat_activity.datname = $1 | ||
| AND pid <> pg_backend_pid() | ||
| `, [dbName]); | ||
|
|
||
| // Small delay to ensure connections are terminated | ||
| await new Promise(r => setTimeout(r, 100)); | ||
|
|
||
| await this.setupClient.query(`DROP DATABASE IF EXISTS "${dbName}"`); | ||
| } catch (err) { | ||
| console.warn(`Failed to drop database ${dbName}:`, err); | ||
| } | ||
| } | ||
| this.databaseNames.clear(); | ||
|
|
||
| try { | ||
| await this.setupClient.end(); | ||
| } catch (err) { | ||
| // Ignore errors when closing setup client | ||
| } | ||
| this.setupClient = null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts | head -200Repository: stack-auth/stack-auth
Length of output: 8492
🏁 Script executed:
cat -n apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts | sed -n '200,400p'Repository: stack-auth/stack-auth
Length of output: 6980
Replace any with unknown and add type guards; type externalDatabases parameter.
Multiple catch blocks use err: any (lines 150, 223, 254, 321) without proper type guards, and createProjectWithExternalDb accepts externalDatabases: any (line 333). Several catch blocks also silently swallow errors (lines 94–96, 117–119, 125–127, 384) without logging, hiding unexpected failures.
Switch catch blocks to err: unknown with a PostgreSQL error guard, and provide a proper type for the externalDatabases config:
🧩 Suggested pattern
+type PgError = { code?: string };
+const isPgError = (err: unknown): err is PgError =>
+ typeof err === 'object' && err !== null && 'code' in err;
-export async function waitForCondition(
- checkFn: () => Promise<boolean>,
- options: { timeoutMs?: number, intervalMs?: number, description?: string } = {}
-): Promise<void> {
+export async function waitForCondition(
+ checkFn: () => Promise<boolean>,
+ options: { timeoutMs?: number, intervalMs?: number, description?: string } = {}
+): Promise<void> {
...
- } catch (err: any) {
- if (err?.code === '57P01' || err?.code === '08006' || err?.code === '53300') {
+ } catch (err: unknown) {
+ if (isPgError(err) && (err.code === '57P01' || err.code === '08006' || err.code === '53300')) {
await new Promise(r => setTimeout(r, intervalMs));
continue;
}
throw err;
}
...
}
-export async function createProjectWithExternalDb(externalDatabases: any, projectOptions?: { display_name?: string, description?: string }) {
+export type ExternalDatabasesConfig = Record<string, unknown>;
+export async function createProjectWithExternalDb(
+ externalDatabases: ExternalDatabasesConfig,
+ projectOptions?: { display_name?: string, description?: string }
+) {Apply the same guard pattern to other catch blocks (lines 223, 254, 321) and remove silent error suppression.
🤖 Prompt for AI Agents
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts` around
lines 47 - 129, Change all catch parameters from any to unknown (e.g., in
TestDbManager.cleanup and other try/catch blocks) and replace silent swallows
with guarded handling: add a type-guard function (e.g., isPostgresError(err:
unknown): err is { code?: string; message?: string }) that checks err is
object/non-null and has message/code properties, then log meaningful details
(console.warn/processLogger.warn) when the guard passes and otherwise rethrow or
log the unknown value; also add a proper type for the externalDatabases
parameter in createProjectWithExternalDb (replace externalDatabases: any with a
defined interface/type describing host, port, user, password, dbName, ssl, etc.)
and apply the same unknown + guard pattern to all other catch blocks mentioned
so no errors are silently ignored.
Comment on lines
+175
to
+199
| for (let attempt = 0; attempt < FORCE_SYNC_ATTEMPTS; attempt++) { | ||
| const sequencerRes = await niceFetch(new URL('/api/latest/internal/external-db-sync/sequencer', STACK_BACKEND_BASE_URL), { | ||
| query: { | ||
| maxDurationMs: String(FORCE_SYNC_MAX_DURATION_MS), | ||
| stopWhenIdle: "true", | ||
| }, | ||
| headers: { | ||
| Authorization: `Bearer ${cronSecret}`, | ||
| }, | ||
| }); | ||
| const pollerRes = await niceFetch(new URL('/api/latest/internal/external-db-sync/poller', STACK_BACKEND_BASE_URL), { | ||
| query: { | ||
| maxDurationMs: String(FORCE_SYNC_MAX_DURATION_MS), | ||
| stopWhenIdle: "true", | ||
| }, | ||
| headers: { | ||
| Authorization: `Bearer ${cronSecret}`, | ||
| }, | ||
| }); | ||
|
|
||
| const sequencerIterations = (sequencerRes.body as { iterations?: number } | undefined)?.iterations ?? 0; | ||
| const requestsProcessed = (pollerRes.body as { requests_processed?: number } | undefined)?.requests_processed ?? 0; | ||
| if (sequencerIterations === 0 && requestsProcessed === 0) { | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate sequencer/poller HTTP status before treating sync as idle.
If either endpoint returns 401/500, iterations/requests_processed default to 0 and the loop exits, masking failures and increasing flake risk. Please throw on non‑2xx responses.
✅ Suggested guard
const sequencerRes = await niceFetch(new URL('/api/latest/internal/external-db-sync/sequencer', STACK_BACKEND_BASE_URL), {
query: {
maxDurationMs: String(FORCE_SYNC_MAX_DURATION_MS),
stopWhenIdle: "true",
},
headers: {
Authorization: `Bearer ${cronSecret}`,
},
});
+ if (sequencerRes.status < 200 || sequencerRes.status >= 300) {
+ throw new Error(`Sequencer sync failed: ${sequencerRes.status}`);
+ }
const pollerRes = await niceFetch(new URL('/api/latest/internal/external-db-sync/poller', STACK_BACKEND_BASE_URL), {
query: {
maxDurationMs: String(FORCE_SYNC_MAX_DURATION_MS),
stopWhenIdle: "true",
},
headers: {
Authorization: `Bearer ${cronSecret}`,
},
});
+ if (pollerRes.status < 200 || pollerRes.status >= 300) {
+ throw new Error(`Poller sync failed: ${pollerRes.status}`);
+ }🤖 Prompt for AI Agents
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.ts` around
lines 175 - 199, The loop currently trusts sequencerRes and pollerRes bodies
without verifying HTTP status, so non‑2xx responses (401/500) will be treated as
idle; update the logic after the niceFetch calls to validate sequencerRes.status
and pollerRes.status (or an isOk/is2xx helper) and throw an error if either
response is not 2xx before reading iterations/requests_processed, referencing
the variables sequencerRes and pollerRes used in this block and keeping the
existing break behavior only when both responses are successful and their counts
are zero.
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
2 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.
Possible CI flake fix: run multiple sequencer/poller passes when forcing sync in tests.\n\n- Lint: pass\n- Typecheck: pass
Summary by CodeRabbit
New Features
Infrastructure