fix(ci): stop shard 5/6 flakes from PostgREST upstream errors - #2808
Conversation
Kong/PostgREST overload on backend shard 5/6 was misread as bad API keys and "no data" app_version creates. Route auth/org seed paths off PostgREST and add a deterministic upstream-outage repro script. 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: 14 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 (5)
📝 WalkthroughWalkthroughThe pull request adds a PostgREST flake reproduction script, moves selected organization and test operations to direct PostgreSQL, prefers PostgreSQL API-key resolution, and maps upstream gateway failures to HTTP 503 responses. ChangesPostgreSQL and PostgREST flake handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReproScript
participant PostgREST
participant PostgreSQL
ReproScript->>PostgREST: Run parallel app-version upserts
PostgREST-->>ReproScript: Return expected failures
ReproScript->>PostgREST: Pause and restore local container
ReproScript->>PostgreSQL: Run equivalent direct SQL upserts
PostgreSQL-->>ReproScript: Return successful writes
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
Use pool.connect() for org name SQL helpers so unit mocks match updateOrg, and stop createAppVersions conflict updates that trip bundle_already_ready. Co-authored-by: Cursor <cursoragent@cursor.com>
drizzle execute returned string ids so authApikey.id === existingApikey.id failed and API keys could rename themselves; also type org SQL helpers with PgTransactionClient for backend typecheck. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Stale comment
Risk: medium. Left a non-blocking comment — Cursor Bugbot reported 1 unresolved finding (
checkKey503 swallowed into 401) and the Bugbot check finished as skipping, so this is not approved. Assigned reviewers for the auth/org SQL path changes.Sent by Cursor Approval Agent: Pull Request Approver
There was a problem hiding this comment.
Stale comment
Risk: medium. Left a non-blocking comment — Cursor Bugbot reported 1 unresolved finding (
checkKey503 swallowed into 401) and the Bugbot check finished as skipping, so this is not approved. Reviewers are already assigned for the auth/org SQL path changes.Sent by Cursor Approval Agent: Pull Request Approver External
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca3039f. Configure here.
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
createDirectApiKeyWithBindings and the read-only organization-api suite setup were still hitting Kong under CF shard load and aborting beforeAll. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/repro-shard5-postgrest-flakes.ts`:
- Around line 147-155: Update the finally block’s dockerAction('unpause',
container) catch handler to log the caught error with clear context, while
retaining the existing cleanup flow and delay so developers are warned when the
container may remain paused.
- Around line 96-107: Wrap the database operation in sqlCreateVersion with
try/catch so pool.query failures return { ok: false, error } instead of
rejecting. Preserve the existing no-data failure result and successful result,
matching the error-reporting behavior of postgrestCreateVersion so runBatch can
continue to cleanupApp and close the pool.
In `@supabase/functions/_backend/public/organization/put.ts`:
- Around line 440-447: The sanitizeOrgNameForSync and getOrgForNameSync
functions each independently call getPgClient(c) to create a new pg.Pool,
resulting in unnecessary connection churn within a single request. Extract a
single pooled client by calling getPgClient once before both function calls,
then update the signatures of sanitizeOrgNameForSync and getOrgForNameSync to
accept the pooled client as a parameter instead of creating their own. Pass this
shared client to both functions so they reuse a single pool for both operations.
In `@supabase/functions/_backend/utils/supabase.ts`:
- Line 1765: The condition checking message.includes('502') and
message.includes('503') is too broad and will match those digit sequences
appearing anywhere in error messages, including in unrelated PostgREST error
details. Replace these substring-only checks with either a check against the
error object's code property if available, or restrict the string match to
require '502' or '503' to appear with explicit status prefixes or in combination
with the 'upstream' context already being checked, to avoid false positives from
digits in identifiers or key names.
- Around line 1762-1771: Update the local catch surrounding the apikey lookup so
the quickError(503, ...) thrown in the upstream-failure branch is re-thrown
instead of converted to null. In the catch block near checkKey, identify and
propagate the HTTP exception using the existing framework error type/import,
while preserving current logging and null handling for genuine lookup failures
so foundAPIKey continues returning invalid_apikey only for invalid keys.
In `@tests/organization-put-stripe-sync.unit.test.ts`:
- Around line 181-190: The strip_html fixture branch must reject unmapped inputs
instead of returning raw values. Update the fixtures lookup in the strip_html
handling to throw a clear error when fixtures[raw] is undefined, while
preserving the existing mapped fixture results.
In `@tests/test-utils.ts`:
- Around line 587-618: The ON CONFLICT DO NOTHING clause causes the fallback
SELECT to silently return pre-existing rows without applying the caller-supplied
field values (deleted, r2_path, checksum, min_update_version, etc.), making
tests run against unexpected data. After the data assignment that follows the
fallback SELECT query, validate that the returned row's field values match the
requested values for all non-trivial fields. If validation fails, throw an error
with details about the mismatch. This ensures the helper fails loudly instead of
proceeding with divergent test data when a row already exists.
- Around line 309-342: Consolidate the hashed and unhashed branches into a
single executeSQL call by using conditional expressions for the key and key_hash
parameter values. Replace the if/else block around the two INSERT statements
with a single INSERT that conditionally passes options.key or NULL and the
digest expression or NULL based on options.hashed, then apply the resulting
insertedKey mapping once after the query completes. This eliminates the
duplicated RETURNING clause and apiKey object mapping logic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3ee2f84-2530-49fe-9322-e2626cb7f5d8
📒 Files selected for processing (8)
scripts/repro-shard5-postgrest-flakes.tssupabase/functions/_backend/public/organization/put.tssupabase/functions/_backend/utils/hono_middleware.tssupabase/functions/_backend/utils/supabase.tstests/organization-api.test.tstests/organization-put-stripe-sync.unit.test.tstests/private-analytics-validation.unit.test.tstests/test-utils.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Direct SQL inserts skip apikeys_force_server_key, so non-UUID secrets were treated as JWTs (401). Match PostgREST+authenticator behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test-utils.ts`:
- Around line 303-305: Update the plainKey assignment in the test utility so
options.key is retained only when it is a valid UUID, regardless of
options.hashed; otherwise generate a random UUID. Ensure the normalized UUID is
both stored and returned for the Authorization header so hashed-key tests reach
database lookup.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7819a30-d1ee-4791-884c-d72f39a78963
📒 Files selected for processing (1)
tests/test-utils.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Outer catch was turning quickError(503) into invalid_apikey 401; also look up API keys on the primary connection to avoid replica lag. Co-authored-by: Cursor <cursoragent@cursor.com>
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_c59b0b16-774e-406b-b1b3-c6649cfba8ab) |
Co-authored-by: Cursor <cursoragent@cursor.com>
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_cae0edbf-a744-42a8-b04f-798a6b9f4d7c) |
Cut remaining Kong/PostgREST setup traffic that still aborted suites under shard load (delete member + audit-logs beforeAll). Co-authored-by: Cursor <cursoragent@cursor.com>
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_7e0f858a-77cc-4404-a285-25e7ae3feaf4) |
|
ci: retrigger |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Restore actionable failure detail after SQL migration of the fixture lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
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_16b7918f-25dc-4d6d-8c15-f56936267edf) |
|






Summary (AI generated)
organization-api(401 instead of 400 / 400 instead of 200) andstats(createAppVersions→ upstream/no data).middlewareAuthAPI-key lookup and org PUT sanitize/lookup through direct Postgres; movecreateAppVersionsoff PostgREST.scripts/repro-shard5-postgrest-flakes.tsthat pauses PostgREST and proves the old path fails 100% while SQL stays green (no test retries).Motivation (AI generated)
Shard 5/6 failed repeatedly across unrelated PRs. Restarts were masking infrastructure errors as app bugs.
Business Impact (AI generated)
Fewer false CI reds on backend shards → faster merges and less wasted engineering time on phantom flakes.
Test Plan (AI generated)
bun run supabase:with-env -- bun scripts/repro-shard5-postgrest-flakes.ts→ PostgREST path 100% fail, SQL path 0% fail (ran 3×)createAppVersionsvia test-utils under load → 50/50 successbun run typecheck:backendGenerated with AI
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Chores