Fix cron jobs using dev env instead of test env in CI workflows#1319
Conversation
The custom-base-port and db-migration-backwards-compatibility workflows were running cron jobs with `with-env:dev` instead of `with-env:test`, causing ClickHouse sync mismatches in verify-data-integrity.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded a pre-verification sync call and adjusted CI cron job startup commands: workflows now run Changes
Sequence Diagram(s)sequenceDiagram
participant Verifier as VerifyScript
participant Syncer as ExternalDBSync
participant ClickHouse as ClickHouseVerifier
participant Postgres as PostgresDB
Verifier->>Syncer: syncExternalDatabases(tenancy)
Syncer-->>Verifier: sync complete
Verifier->>ClickHouse: verifyClickhouseSync(tenancy)
ClickHouse->>Postgres: query expected rows
ClickHouse->>ClickHouse: compare ordered rows (SORT_KEYS)
ClickHouse-->>Verifier: verification result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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 |
Greptile SummaryThis PR fixes three CI workflow steps that were running cron jobs with the Confidence Score: 5/5Safe to merge — targeted, correct fix with no logic issues. All three changed lines are straightforward environment-name corrections that align the affected workflows with the already-working No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI Workflow Triggered] --> B[Build & Install Dependencies]
B --> C[Copy .env.development → .env.test.local]
C --> D[Start Backend / Dashboard / OAuth Server]
D --> E[Start run-email-queue\npnpm run run-email-queue\n uses with-env:dev]
D --> F["Start run-cron-jobs\n(BEFORE: with-env:dev)\n(AFTER: run-cron-jobs:test → with-env:test)"]
E --> G[Run Tests]
F --> G
G --> H[verify-data-integrity]
H --> I{ClickHouse in sync?}
I -- "BEFORE fix: NO\n(dev env ≠ test env)" --> J[❌ Failure]
I -- "AFTER fix: YES\n(both use test env)" --> K[✅ Pass]
Reviews (1): Last reviewed commit: "Fix cron jobs using dev env instead of t..." | Re-trigger Greptile |
Two issues: 1. verify-data-integrity runs before all QStash sync callbacks have been delivered, causing stale data in ClickHouse. Fix by calling syncExternalDatabases() directly before verifying each tenancy. 2. notification_preferences sort keys used ["id"] but the ClickHouse table has no id column — it uses (user_id, notification_category_id) as the composite key. Fix sort keys to match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/verify-data-integrity/index.ts`:
- Around line 232-235: syncExternalDatabases(tenancy) currently has its return
value ignored which lets verify run despite pending/throttled sync work; update
the call site to inspect the returned flag (e.g., needsResync) and implement a
bounded retry loop (with a small backoff and max attempts) that re-invokes
syncExternalDatabases and only proceeds to ClickHouse verification when it
returns false, otherwise fail fast by throwing an error after exhausting
retries; reference the syncExternalDatabases return handling and the subsequent
ClickHouse verification call so the change affects the awaiting logic before
verification.
🪄 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: d5a8d561-d394-493a-9b98-00db7cd59589
📒 Files selected for processing (2)
apps/backend/scripts/verify-data-integrity/clickhouse-sync-verifier.tsapps/backend/scripts/verify-data-integrity/index.ts
| // Flush any pending ClickHouse syncs by running a direct sync before verifying. | ||
| // This avoids race conditions where QStash hasn't delivered all sync callbacks yet. | ||
| await syncExternalDatabases(tenancy); | ||
|
|
There was a problem hiding this comment.
Handle syncExternalDatabases() return value before verifying ClickHouse.
Line 234 currently ignores the needsResync signal, so verification can still run with pending/throttled sync work.
🔧 Proposed fix (bounded retry + fail fast)
await recurse("[clickhouse sync]", async (recurse) => {
// Flush any pending ClickHouse syncs by running a direct sync before verifying.
// This avoids race conditions where QStash hasn't delivered all sync callbacks yet.
- await syncExternalDatabases(tenancy);
+ const MAX_SYNC_ATTEMPTS = 5;
+ for (let attempt = 1; attempt <= MAX_SYNC_ATTEMPTS; attempt++) {
+ const needsResync = await syncExternalDatabases(tenancy);
+ if (!needsResync) break;
+ if (attempt === MAX_SYNC_ATTEMPTS) {
+ throw new StackAssertionError(
+ `ClickHouse sync still pending after ${MAX_SYNC_ATTEMPTS} attempts for project ${projectId}.`
+ );
+ }
+ await wait(250);
+ }
await verifyClickhouseSync({
tenancy,
projectId,
branchId: DEFAULT_BRANCH_ID,As per coding guidelines: "Fail early, fail loud; fail fast with an error instead of silently continuing" and "Validate all assumptions through type system (preferred), assertions, or tests; ideally two out of three".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/scripts/verify-data-integrity/index.ts` around lines 232 - 235,
syncExternalDatabases(tenancy) currently has its return value ignored which lets
verify run despite pending/throttled sync work; update the call site to inspect
the returned flag (e.g., needsResync) and implement a bounded retry loop (with a
small backoff and max attempts) that re-invokes syncExternalDatabases and only
proceeds to ClickHouse verification when it returns false, otherwise fail fast
by throwing an error after exhausting retries; reference the
syncExternalDatabases return handling and the subsequent ClickHouse verification
call so the change affects the awaiting logic before verification.
1 participant
The custom-base-port and db-migration-backwards-compatibility workflows were running cron jobs with
with-env:devinstead ofwith-env:test, causing ClickHouse sync mismatches in verify-data-integrity.Summary by CodeRabbit