ci: improve temp replica diagnostics#2745
Conversation
CREATE/DROP/REINDEX CONCURRENTLY cannot run inside BEGIN/COMMIT, so the read-replica schema sync rejected plain additive indexes and blocked deploy. Co-authored-by: Cursor <cursoragent@cursor.com>
Apply columns/types/sequences, then non-CONCURRENTLY index DDL, then USING INDEX attaches. Accept quoted index names and restore config.toml. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep enforce_encrypted_bundle_trigger disabled only inside each short per-version txn, and strip CONCURRENTLY only from leading index DDL. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4c218e14-e527-41b0-b7e9-7fab2ab3a57d) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: medium. Cursor Bugbot did not complete (skipped due to usage limit), so the required automated review signal is missing. Human review is needed for the replica sync script and CI workflow changes; no reviewers were assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 4 files
Confidence score: 2/5
- In
scripts/sync-read-replica-schema.ts, switching large index creation away from concurrent mode can block writes on the subscriber and stall logical-replication replay during deployment, creating a direct availability/regression risk — restore concurrent index creation (or route this set through an import path that supports it). - In
scripts/reclaim_deleted_version_manifests.ts, per-candidate cleanup now takes anACCESS EXCLUSIVElock onpublic.app_versions, so routine version operations can be blocked repeatedly while the job runs — reduce lock scope/duration (or refactor batching/ordering) to avoid table-wide blocking. - In
.github/workflows/replica-diag.yml, missingtimeout-minutesplus non-quietgcloudcalls can let a single prompt/hang stall CI indefinitely, delaying incident response and deploy flow — addtimeout-minutesand--quieton gcloud invocations. - In
.github/workflows/replica-diag.yml, SQL diagnostic commands can fail while the workflow still reports success, which hides replica health issues from operators — keep optional logging best-effort but makeexecute-sqlfailures surface in job status; then tightentests/read-replica-schema-import-sync.unit.test.tswith an uppercase quotedCONCURRENTLYcase to validate the intended guard.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/replica-diag.yml">
<violation number="1" location=".github/workflows/replica-diag.yml:6">
P2: This job is missing a `timeout-minutes` setting, unlike every other workflow in this repository. With eight `gcloud sql instances execute-sql` calls and several other gcloud commands, a single hang against an unresponsive Cloud SQL instance could waste a runner for up to the 360-minute default. Adding `timeout-minutes: 15` (or similar) would match the project convention and limit resource waste.</violation>
<violation number="2" location=".github/workflows/replica-diag.yml:21">
P2: The gcloud commands in this workflow don't use `--quiet`. In a non-interactive CI runner, gcloud may prompt for confirmation in certain conditions (first-use notices, quota warnings, etc.), which would hang the step. Adding `--quiet` to each gcloud call (or setting `CLOUDSDK_CORE_DISABLE_PROMPTS=1` at the env level) is the standard pattern for CI/CD gcloud usage and prevents unexpected hangs beyond the `|| true` fallback.</violation>
<violation number="3" location=".github/workflows/replica-diag.yml:41">
P2: Replica SQL diagnostics can fail while this workflow reports success, so operators cannot distinguish a healthy replica from missing diagnostic output. Keep the logging read best-effort as intended, but let `execute-sql` failures fail the job (or collect and return a nonzero status after all queries).</violation>
</file>
<file name="tests/read-replica-schema-import-sync.unit.test.ts">
<violation number="1" location="tests/read-replica-schema-import-sync.unit.test.ts:81">
P3: The `tricky` case does not exercise the claimed protection against replacing `CONCURRENTLY` inside a quoted identifier because the embedded occurrence is lowercase. Using uppercase `CONCURRENTLY` in the quoted name would make this test catch an unanchored replacement.</violation>
</file>
<file name="scripts/sync-read-replica-schema.ts">
<violation number="1" location="scripts/sync-read-replica-schema.ts:524">
P1: Large index builds now block writes on the live subscriber, which can stall logical-replication replay during deployment. Keep index creation concurrent, or run this group through an import path that supports concurrent DDL instead of stripping the keyword.</violation>
</file>
<file name="scripts/reclaim_deleted_version_manifests.ts">
<violation number="1" location="scripts/reclaim_deleted_version_manifests.ts:201">
P1: Each cleanup iteration now holds an `ACCESS EXCLUSIVE` lock on the entire `public.app_versions` table while deleting the version's manifest rows and updating counters. Because this runs once per candidate, normal version reads and writes can be blocked repeatedly for the duration of the reclaim job; a session-local trigger bypass (or another approach that avoids per-version `ALTER TABLE`) would preserve the diagnostic workaround without taking a table-wide lock around the data work.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| function renderCloudSqlIndexStatement(sql: string): string { | ||
| return sql | ||
| .replace(/^CREATE UNIQUE INDEX CONCURRENTLY /u, 'CREATE UNIQUE INDEX ') |
There was a problem hiding this comment.
P1: Large index builds now block writes on the live subscriber, which can stall logical-replication replay during deployment. Keep index creation concurrent, or run this group through an import path that supports concurrent DDL instead of stripping the keyword.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/sync-read-replica-schema.ts, line 524:
<comment>Large index builds now block writes on the live subscriber, which can stall logical-replication replay during deployment. Keep index creation concurrent, or run this group through an import path that supports concurrent DDL instead of stripping the keyword.</comment>
<file context>
@@ -370,12 +484,49 @@ export function renderReadReplicaImportTransaction(
+
+function renderCloudSqlIndexStatement(sql: string): string {
+ return sql
+ .replace(/^CREATE UNIQUE INDEX CONCURRENTLY /u, 'CREATE UNIQUE INDEX ')
+ .replace(/^CREATE INDEX CONCURRENTLY /u, 'CREATE INDEX ')
+ .replace(/^DROP INDEX CONCURRENTLY /u, 'DROP INDEX ')
</file context>
| updated_at = now() | ||
| WHERE app_id = $1`, | ||
| [version.app_id], | ||
| `ALTER TABLE public.app_versions DISABLE TRIGGER ${LOCK_TRIGGER}`, |
There was a problem hiding this comment.
P1: Each cleanup iteration now holds an ACCESS EXCLUSIVE lock on the entire public.app_versions table while deleting the version's manifest rows and updating counters. Because this runs once per candidate, normal version reads and writes can be blocked repeatedly for the duration of the reclaim job; a session-local trigger bypass (or another approach that avoids per-version ALTER TABLE) would preserve the diagnostic workaround without taking a table-wide lock around the data work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/reclaim_deleted_version_manifests.ts, line 201:
<comment>Each cleanup iteration now holds an `ACCESS EXCLUSIVE` lock on the entire `public.app_versions` table while deleting the version's manifest rows and updating counters. Because this runs once per candidate, normal version reads and writes can be blocked repeatedly for the duration of the reclaim job; a session-local trigger bypass (or another approach that avoids per-version `ALTER TABLE`) would preserve the diagnostic workaround without taking a table-wide lock around the data work.</comment>
<file context>
@@ -162,68 +172,85 @@ async function main() {
- updated_at = now()
- WHERE app_id = $1`,
- [version.app_id],
+ `ALTER TABLE public.app_versions DISABLE TRIGGER ${LOCK_TRIGGER}`,
)
+
</file context>
| gcloud sql instances execute-sql eu-2 \ | ||
| --database=postgres \ | ||
| --sql="SELECT subname, pid, received_lsn, latest_end_lsn, last_msg_send_time, last_msg_receipt_time, now()-last_msg_receipt_time AS no_message_for FROM pg_stat_subscription;" \ | ||
| --format=json || true |
There was a problem hiding this comment.
P2: Replica SQL diagnostics can fail while this workflow reports success, so operators cannot distinguish a healthy replica from missing diagnostic output. Keep the logging read best-effort as intended, but let execute-sql failures fail the job (or collect and return a nonzero status after all queries).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/replica-diag.yml, line 41:
<comment>Replica SQL diagnostics can fail while this workflow reports success, so operators cannot distinguish a healthy replica from missing diagnostic output. Keep the logging read best-effort as intended, but let `execute-sql` failures fail the job (or collect and return a nonzero status after all queries).</comment>
<file context>
@@ -0,0 +1,71 @@
+ gcloud sql instances execute-sql eu-2 \
+ --database=postgres \
+ --sql="SELECT subname, pid, received_lsn, latest_end_lsn, last_msg_send_time, last_msg_receipt_time, now()-last_msg_receipt_time AS no_message_for FROM pg_stat_subscription;" \
+ --format=json || true
+
+ echo "==== pg_subscription ===="
</file context>
| credentials_file="$(mktemp)" | ||
| trap 'rm -f "$credentials_file"' EXIT | ||
| printf '%s' "$GOOGLE_SERVICE_ACCOUNT_BASE64" | base64 --decode > "$credentials_file" | ||
| gcloud auth activate-service-account --key-file="$credentials_file" |
There was a problem hiding this comment.
P2: The gcloud commands in this workflow don't use --quiet. In a non-interactive CI runner, gcloud may prompt for confirmation in certain conditions (first-use notices, quota warnings, etc.), which would hang the step. Adding --quiet to each gcloud call (or setting CLOUDSDK_CORE_DISABLE_PROMPTS=1 at the env level) is the standard pattern for CI/CD gcloud usage and prevents unexpected hangs beyond the || true fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/replica-diag.yml, line 21:
<comment>The gcloud commands in this workflow don't use `--quiet`. In a non-interactive CI runner, gcloud may prompt for confirmation in certain conditions (first-use notices, quota warnings, etc.), which would hang the step. Adding `--quiet` to each gcloud call (or setting `CLOUDSDK_CORE_DISABLE_PROMPTS=1` at the env level) is the standard pattern for CI/CD gcloud usage and prevents unexpected hangs beyond the `|| true` fallback.</comment>
<file context>
@@ -0,0 +1,71 @@
+ credentials_file="$(mktemp)"
+ trap 'rm -f "$credentials_file"' EXIT
+ printf '%s' "$GOOGLE_SERVICE_ACCOUNT_BASE64" | base64 --decode > "$credentials_file"
+ gcloud auth activate-service-account --key-file="$credentials_file"
+ gcloud config set project capgo-394818
+
</file context>
| gcloud auth activate-service-account --key-file="$credentials_file" | |
| gcloud auth activate-service-account --key-file="$credentials_file" --quiet |
| @@ -0,0 +1,71 @@ | |||
| name: Replica diag (temp) | |||
There was a problem hiding this comment.
P2: This job is missing a timeout-minutes setting, unlike every other workflow in this repository. With eight gcloud sql instances execute-sql calls and several other gcloud commands, a single hang against an unresponsive Cloud SQL instance could waste a runner for up to the 360-minute default. Adding timeout-minutes: 15 (or similar) would match the project convention and limit resource waste.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/replica-diag.yml, line 6:
<comment>This job is missing a `timeout-minutes` setting, unlike every other workflow in this repository. With eight `gcloud sql instances execute-sql` calls and several other gcloud commands, a single hang against an unresponsive Cloud SQL instance could waste a runner for up to the 360-minute default. Adding `timeout-minutes: 15` (or similar) would match the project convention and limit resource waste.</comment>
<file context>
@@ -0,0 +1,71 @@
+on:
+ workflow_dispatch:
+
+jobs:
+ diag:
+ runs-on: ubuntu-latest
</file context>
| name: Replica diag (temp) | |
| jobs: | |
| diag: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| permissions: | |
| contents: read |
| name: 'create index concurrently trap', | ||
| sql: 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "create index concurrently trap" ON public."apps" ("app_id")', | ||
| } | ||
| assertGoogleReadReplicaSchemaPlan(plan([tricky])) | ||
| expect(renderReadReplicaIndexImport([tricky])).toBe( | ||
| 'CREATE INDEX IF NOT EXISTS "create index concurrently trap" ON public."apps" ("app_id");', |
There was a problem hiding this comment.
P3: The tricky case does not exercise the claimed protection against replacing CONCURRENTLY inside a quoted identifier because the embedded occurrence is lowercase. Using uppercase CONCURRENTLY in the quoted name would make this test catch an unanchored replacement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/read-replica-schema-import-sync.unit.test.ts, line 81:
<comment>The `tricky` case does not exercise the claimed protection against replacing `CONCURRENTLY` inside a quoted identifier because the embedded occurrence is lowercase. Using uppercase `CONCURRENTLY` in the quoted name would make this test catch an unanchored replacement.</comment>
<file context>
@@ -40,14 +63,57 @@ describe('read-replica Cloud SQL server-side import', () => {
+ const tricky: ReadReplicaSchemaSyncStatement = {
+ kind: 'index',
+ table: 'apps',
+ name: 'create index concurrently trap',
+ sql: 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "create index concurrently trap" ON public."apps" ("app_id")',
+ }
</file context>
| name: 'create index concurrently trap', | |
| sql: 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "create index concurrently trap" ON public."apps" ("app_id")', | |
| } | |
| assertGoogleReadReplicaSchemaPlan(plan([tricky])) | |
| expect(renderReadReplicaIndexImport([tricky])).toBe( | |
| 'CREATE INDEX IF NOT EXISTS "create index concurrently trap" ON public."apps" ("app_id");', | |
| name: 'create index CONCURRENTLY trap', | |
| sql: 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "create index CONCURRENTLY trap" ON public."apps" ("app_id")', | |
| } | |
| assertGoogleReadReplicaSchemaPlan(plan([tricky])) | |
| expect(renderReadReplicaIndexImport([tricky])).toBe( | |
| 'CREATE INDEX IF NOT EXISTS "create index CONCURRENTLY trap" ON public."apps" ("app_id");', |
1 participant
AI generated: continue past logging deny; run execute-sql
Made with Cursor