You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PG lifecycle = Cloudflare Hyperdrive contract: new pg.Client per request, await connect(), no end() (connection lifecycle).
Manifest deferral with load-test evidence: channel queries skip json_agg. New-version path uses indexed manifest WHERE app_version_id = ?, started right after up-to-date check and overlapped with signed URL work.
Local Postgres A/B load bench (bun run bench:updates-manifest-path): at 5k files/version, 205k rows, conc=50, new_version p95 −34% vs old json_agg; up_to_date p95 −96%. See scripts/bench/updates_manifest_path_summary.md.
Motivation (AI generated)
Memory sawtooth + stuck P999. Deferring manifest must not punish devices that get a bundle. Guessing is not allowed — measured under concurrent load against a selective idx_manifest_app_version_id.
Business Impact (AI generated)
Keeps delivery path healthy while making the common up-to-date check ~20× cheaper on the DB critical path.
Test Plan (AI generated)
bunx vitest run tests/plugin-pg-client-lifecycle.unit.test.ts
bun run typecheck:backend
bun run bench:updates-manifest-path (PASS gate: new_version not destroyed)
Run tests CI on prior tips; re-verify this tip
After deploy: compare plugin_path_timingoutcome=new_version vs baseline
Load test evidence (AI generated)
Scenario (5k files)
OLD p95
NEW p95
Delta
new_version
224.2 ms
147.7 ms
−34%
up_to_date
217.1 ms
8.4 ms
−96%
EXPLAIN: deferred path uses Index Scan on idx_manifest_app_version_id.
Generated with AI
Summary by CodeRabbit
Performance
Improved response responsiveness by deferring non-critical recording work in the channel self flow.
Reduced unnecessary database work by skipping manifest loading when explicitly disabled.
Deferred manifest fetching when needed and overlapped related work to minimize request latency.
Reliability
Improved PostgreSQL client lifecycle management across runtimes, with safer cleanup and error-tolerant shutdown.
Observability
Added structured timing metrics for update handling and key steps when thresholds are met.
Testing
Added unit coverage for PostgreSQL client lifecycle behavior.
Bug Fixes
Ensured Postgres client acquisition is consistently awaited across affected handlers/utilities.
Fixed onboarding reminder test expectation by updating a missing argument.
Reuse one pg.Pool per Hyperdrive connection on workerd (closeClient was a
no-op while every request allocated a new Pool). Defer /updates manifest
json_agg until after the up-to-date short-circuit, background channel_self
rate-limit writes, and add plugin_path_timing logs for P999 triage.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
The plugin runtime now supports runtime-aware PostgreSQL clients, awaits client acquisition across handlers, hardens notification cleanup, defers manifest loading, bounds response-feature caching, and records timing data for slow update paths.
Hyperdrive clients and non-Hyperdrive pools use separate lifecycle paths, cleanup runs in the background, and lifecycle tests cover connection and failure behavior.
…is 0
App-level manifest_bundle_count already gates the fetch; requiring
version.manifest_count skipped real manifest rows when that column lagged.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
Local Miniflare sets CAPGO_PREVENT_BACKGROUND_FUNCTIONS and runs many
concurrent requests on one isolate; a shared max:4 Pool hit the 10s
connect timeout. Keep isolate pool reuse for production only.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is needed for the plugin-runtime PG pool and /updates manifest changes; reviewers assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Cursor Bugbot did not complete (skipped due to usage limit), so automated review is incomplete. Human review is needed for this plugin hot-path change; reviewers assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 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 `@supabase/functions/_backend/plugin_runtime/utils/pg.ts`:
- Around line 368-369: Update getWorkerdPgPoolKey and its callers so the
shared-pool key includes the request-derived application_name alongside the
connection string and read-only options. Ensure pool lookup and creation use the
same complete key, preventing different application names from reusing one pool.
- Around line 1014-1016: Refactor the shouldFetchManifest assignment to remove
the nested ternary while preserving its current behavior: return false when
includeManifest is explicitly false, otherwise return true when
manifestBundleCount is null or undefined or greater than zero. Use a clear
boolean expression in place of the conditional nesting.
In `@supabase/functions/_backend/plugin_runtime/utils/update.ts`:
- Around line 460-466: Update the missing-bundle validation around the
version-specific lazy manifest fetch so that, after the query completes with no
manifest entries, a selected version lacking both r2_path and manifest rows
still follows the existing no_bundle error branch. Do not rely solely on the
positive app-level manifestBundleCount; preserve the existing no_bundle_url
behavior only for versions with an actual bundle URL or applicable
internal-version handling.
🪄 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: 42d22f25-fcc6-4992-821b-29c40d38e443
📥 Commits
Reviewing files that changed from the base of the PR and between 9eb4e29 and 3805200.
Use a stable shared-pool application_name/key, simplify shouldFetchManifest,
restore no_bundle after empty deferred manifest fetch, and assert pools stay
isolated per connection string.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is needed for the plugin-runtime PG pool and /updates manifest changes; reviewers already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Cursor Bugbot did not complete (skipped due to usage limit), so automated review is incomplete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
…reuse
Cloudflare Hyperdrive docs: create a new client each request; Hyperdrive
owns the DB pool. Shared isolate Pool reuse was an unproven bet at plugin
scale. Use Pool(max:1) per request (sync API) and always end via waitUntil.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving: Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for the plugin-runtime PG pool and /updates manifest changes; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete, and this plugin hot-path change exceeds the low-risk approval threshold. Human review is still needed; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
Address review: close per-call Pools in sendEmailToOrgMembers, keep
getPgClient inside try/finally for notification helpers, and assert
Hyperdrive Client is not returned before connect() resolves.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot did not complete (skipped due to usage limit), and this plugin hot-path change exceeds the low-risk approval threshold. Human review is still needed; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
Point getPgClient/closeClient comments at the Cloudflare page that
explicitly says client.end() is not needed for Hyperdrive.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete, and this plugin hot-path change exceeds the low-risk approval threshold. Human review is still needed; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
Deferred record() correctly matches the P999 objective; drop the now-redundant as any cast.
pgClient is now properly typed as PluginPgClient, so getDrizzleClient(pgClient as any, ...) no longer needs the cast — keeping it risks silently hiding future type mismatches.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/functions/_backend/plugin_runtime/plugins/channel_self.ts` around
lines 636 - 652, Remove the redundant as any cast from the getDrizzleClient call
in runChannelSelfWithPgClient, passing the properly typed pgClient directly
while preserving the existing logger configuration and cleanup flow.
closeClient doesn't guard against a synchronous throw from db.end().
Promise.resolve(db.end()).catch(...) only catches a rejected promise. If db.end() throws synchronously (e.g., double-end() on some pg internal states), the throw escapes before .catch() is attached, and closeClient — called from many finally blocks — would itself throw and mask the original error/response. Wrap the call in an async IIFE so both sync and async failures are caught.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/functions/_backend/plugin_runtime/utils/pg.ts` around lines 503 -
517, Update closeClient so the db.end() invocation occurs inside an async IIFE
or equivalent try/catch boundary, ensuring both synchronous throws and promise
rejections reach the existing cloudlogErr handler. Preserve the skipEndClients
early return, backgroundTask usage, request ID, and existing error message.
Eight admin analytics functions in pg.ts still leak their PostgreSQL client — the exact bug class this PR fixes elsewhere in the same file.
Each of these functions now does pgClient = await getPgClient(c, ...) but has no finally { await closeClient(c, pgClient) }. On any runtime path that resolves to a Pool (non-workerd Supabase Edge Functions, or workerd without a matching Hyperdrive binding for that DB URL), every call to these functions leaks a connection pool indefinitely. Sibling functions in this same file (getAdminOrganizationInsights, getAdminTrialPlanBreakdown, getLiveRegisteredUsersCount, getAdminPayingOrgBreakdown) were correctly fixed with the let pgClient: PluginPgClient | undefined + finally { if (pgClient) await closeClient(c, pgClient) } pattern — apply the same pattern to all sites below.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L1559-L1598: wrap getAdminDeploymentsTrend's body in try { ... } finally { if (pgClient) await closeClient(c, pgClient) }, hoisting pgClient to a let declared before the try.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L1697-L2003: apply the same fix to getAdminGlobalStatsTrend; note its catch currently rethrows (throw e), so the client leaks on both success and failure.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L2164-L2273: apply the same fix to getAdminEmailTypeBreakdown.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L2284-L2356: apply the same fix to getAdminCustomerCountryBreakdown.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L2849-L2937: apply the same fix to getAdminCancelledOrganizations.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L2965-L3052: apply the same fix to getAdminTrialOrganizations.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L3207-L3463: apply the same fix to getAdminOnboardingFunnel.
supabase/functions/_backend/plugin_runtime/utils/pg.ts#L3465-L3546: apply the same fix to getAdminPluginBreakdown.
🤖 Prompt for 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.
In `@supabase/functions/_backend/plugin_runtime/utils/pg.ts` around lines 1559 -
1598, Admin analytics functions leak PostgreSQL clients because they do not
close clients on every exit path. In
supabase/functions/_backend/plugin_runtime/utils/pg.ts, update
getAdminDeploymentsTrend (1559-1598), getAdminGlobalStatsTrend (1697-2003),
getAdminEmailTypeBreakdown (2164-2273), getAdminCustomerCountryBreakdown
(2284-2356), getAdminCancelledOrganizations (2849-2937),
getAdminTrialOrganizations (2965-3052), getAdminOnboardingFunnel (3207-3463),
and getAdminPluginBreakdown (3465-3546) by hoisting pgClient as let pgClient:
PluginPgClient | undefined before each try, assigning it inside, and adding
finally cleanup with closeClient when defined; preserve existing catch behavior,
including rethrowing in getAdminGlobalStatsTrend.
🤖 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.
Outside diff comments:
In `@supabase/functions/_backend/plugin_runtime/plugins/channel_self.ts`:
- Around line 636-652: Remove the redundant as any cast from the
getDrizzleClient call in runChannelSelfWithPgClient, passing the properly typed
pgClient directly while preserving the existing logger configuration and cleanup
flow.
In `@supabase/functions/_backend/plugin_runtime/utils/pg.ts`:
- Around line 503-517: Update closeClient so the db.end() invocation occurs
inside an async IIFE or equivalent try/catch boundary, ensuring both synchronous
throws and promise rejections reach the existing cloudlogErr handler. Preserve
the skipEndClients early return, backgroundTask usage, request ID, and existing
error message.
- Around line 1559-1598: Admin analytics functions leak PostgreSQL clients
because they do not close clients on every exit path. In
supabase/functions/_backend/plugin_runtime/utils/pg.ts, update
getAdminDeploymentsTrend (1559-1598), getAdminGlobalStatsTrend (1697-2003),
getAdminEmailTypeBreakdown (2164-2273), getAdminCustomerCountryBreakdown
(2284-2356), getAdminCancelledOrganizations (2849-2937),
getAdminTrialOrganizations (2965-3052), getAdminOnboardingFunnel (3207-3463),
and getAdminPluginBreakdown (3465-3546) by hoisting pgClient as let pgClient:
PluginPgClient | undefined before each try, assigning it inside, and adding
finally cleanup with closeClient when defined; preserve existing catch behavior,
including rethrowing in getAdminGlobalStatsTrend.
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1fb8bba-0ae7-47b1-8ed5-13cd6aa31d16
📥 Commits
Reviewing files that changed from the base of the PR and between e33cbaa and 22c0f80.
Protect the path that returns files: start the indexed
manifest.app_version_id query as soon as we know the device is not
up-to-date, and await it in parallel with signed URL generation.
Keep channel-query json_agg off so up-to-date stays cheap.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete, and this plugin hot-path change exceeds the low-risk approval threshold. Human review is still needed; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
The reason will be displayed to describe this comment to others. Learn more.
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 `@supabase/functions/_backend/plugin_runtime/utils/update.ts`:
- Around line 502-514: Update deferredManifestPromise in the
needsDeferredManifest branch to call execute() on the lazy
requestManifestEntriesPostgres query, creating one native promise. Reuse that
promise for the existing early catch and later Promise.all flow so the manifest
lookup executes only once.
🪄 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: 78790589-5cfb-4950-a44c-d02a55df522b
📥 Commits
Reviewing files that changed from the base of the PR and between 22c0f80 and a846807.
Reuse a native promise for the deferred manifest query.
requestManifestEntriesPostgres(...) returns a lazy Drizzle query object, so the early .catch(() => {}) and the later Promise.all([...]) each re-run the query. That doubles the manifest lookup on the hot path and defeats the overlap optimization. Call .execute() when creating deferredManifestPromise, then reuse that promise for both places.
🤖 Prompt for 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.
In `@supabase/functions/_backend/plugin_runtime/utils/update.ts` around lines 502
- 514, Update deferredManifestPromise in the needsDeferredManifest branch to
call execute() on the lazy requestManifestEntriesPostgres query, creating one
native promise. Reuse that promise for the existing early catch and later
Promise.all flow so the manifest lookup executes only once.
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Confidence score: 3/5
In supabase/functions/_backend/plugin_runtime/utils/update.ts, the Drizzle query builder is being consumed via .catch(() => {}) and then awaited later, which can execute the same SQL twice because the builder is thenable; that creates real regression risk like duplicate updates/inserts and harder-to-trace behavior from swallowed errors — execute the query exactly once (store/await once) and handle errors on that single execution path.
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="supabase/functions/_backend/plugin_runtime/utils/update.ts">
<violation number="1" location="supabase/functions/_backend/plugin_runtime/utils/update.ts:514">
P1: Drizzle query builders are thenables — they re-execute the underlying SQL each time `.then()`, `.catch()`, or `await` is invoked on them. Here, `.catch(() => {})` on line 514 triggers the first execution, and the later `Promise.all([..., deferredManifestPromise])` triggers a second execution because it's still the raw thenable, not a settled native Promise.
To execute the query exactly once and reuse the result, convert to a native promise eagerly:
```ts
const deferredManifestPromise = needsDeferredManifest
? Promise.resolve(requestManifestEntriesPostgres(c, version.id, drizzleClient))
: null
Wrapping with Promise.resolve(thenable) subscribes once and returns a native Promise that can be safely .catch()-ed and later awaited in Promise.all without re-running the query.
</details>
<sub>**Tip**: Review your code locally with the [cubic CLI](https://docs.cubic.dev/ide/cli-review?utm_source=github&utm_content=general_review_body) to iterate faster.<br /><br />[Re-trigger cubic](https://www.cubic.dev/action/re-review/pr/Cap-go/capgo.app/2741/ai_pr_review_1784923562221_b3939997-282c-45a4-aede-4b7177fc32f8?returnTo=https%3A%2F%2Fgithub.com%2FCap-go%2Fcapgo.app%2Fpull%2F2741)</sub>
<!-- cubic:review-post:ai_pr_review_1784923562221_b3939997-282c-45a4-aede-4b7177fc32f8:a84680749c7b3262df586b559db22a3edc362733:0cb8ba32-7f6d-46fc-bbe8-0aa4ee97e059 -->
The reason will be displayed to describe this comment to others. Learn more.
P1: Drizzle query builders are thenables — they re-execute the underlying SQL each time .then(), .catch(), or await is invoked on them. Here, .catch(() => {}) on line 514 triggers the first execution, and the later Promise.all([..., deferredManifestPromise]) triggers a second execution because it's still the raw thenable, not a settled native Promise.
To execute the query exactly once and reuse the result, convert to a native promise eagerly:
Wrapping with Promise.resolve(thenable) subscribes once and returns a native Promise that can be safely .catch()-ed and later awaited in Promise.all without re-running the query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/functions/_backend/plugin_runtime/utils/update.ts, line 514:
<comment>Drizzle query builders are thenables — they re-execute the underlying SQL each time `.then()`, `.catch()`, or `await` is invoked on them. Here, `.catch(() => {})` on line 514 triggers the first execution, and the later `Promise.all([..., deferredManifestPromise])` triggers a second execution because it's still the raw thenable, not a settled native Promise.
To execute the query exactly once and reuse the result, convert to a native promise eagerly:
```ts
const deferredManifestPromise = needsDeferredManifest
? Promise.resolve(requestManifestEntriesPostgres(c, version.id, drizzleClient))
: null
Wrapping with Promise.resolve(thenable) subscribes once and returns a native Promise that can be safely .catch()-ed and later awaited in Promise.all without re-running the query.
@@ -499,6 +499,20 @@ export async function updateWithPG(
+ ? requestManifestEntriesPostgres(c, version.id, drizzleClient)
+ : null
+ // If a gate returns early, this in-flight query must not become an unhandled rejection.
+ deferredManifestPromise?.catch(() => {})
+
if (channelData) {
```
Add a local Postgres A/B load test for the /updates manifest path.
At 5k files/version (205k rows, conc=50): new_version p95 improved
34% and up_to-date p95 improved 96%. Gate fails CI-style if
new_version regresses. Commit results + summary for PR evidence.
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete, and this plugin hot-path change exceeds the low-risk approval threshold. Human review is still needed; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver External
The reason will be displayed to describe this comment to others. Learn more.
Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached), so automated review did not complete. Human review is still needed for this plugin hot-path change; reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Approver
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files (changes from recent commits).
Confidence score: 2/5
In scripts/bench_updates_manifest_path.ts, the benchmark setup can use the app DATABASE_URL and drop the public schema, which creates a concrete data-loss risk if someone runs it in a non-isolated environment; switch to a dedicated benchmark/replica-only connection variable and add an explicit guard that refuses known app/staging/production URLs before any destructive step.
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="scripts/bench_updates_manifest_path.ts">
<violation number="1" location="scripts/bench_updates_manifest_path.ts:330">
P0: Running this script in an environment with the application `DATABASE_URL` destroys that database's `public` schema before benchmarking. Use a dedicated benchmark/replica-only connection variable plus a guard that rejects non-benchmark targets, rather than accepting the general runtime DB URL.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
The reason will be displayed to describe this comment to others. Learn more.
P0: Running this script in an environment with the application DATABASE_URL destroys that database's public schema before benchmarking. Use a dedicated benchmark/replica-only connection variable plus a guard that rejects non-benchmark targets, rather than accepting the general runtime DB URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/bench_updates_manifest_path.ts, line 330:
<comment>Running this script in an environment with the application `DATABASE_URL` destroys that database's `public` schema before benchmarking. Use a dedicated benchmark/replica-only connection variable plus a guard that rejects non-benchmark targets, rather than accepting the general runtime DB URL.</comment>
<file context>
@@ -0,0 +1,506 @@
+}
+
+async function main() {
+ const databaseUrl = process.env.DATABASE_URL || DEFAULT_URL
+ const files = Number(argValue('--files', '5000'))
+ const concurrency = Number(argValue('--concurrency', '50'))
</file context>
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
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Summary (AI generated)
pg.Clientper request,await connect(), noend()(connection lifecycle).json_agg. New-version path uses indexedmanifest WHERE app_version_id = ?, started right after up-to-date check and overlapped with signed URL work.bun run bench:updates-manifest-path): at 5k files/version, 205k rows, conc=50, new_version p95 −34% vs old json_agg; up_to_date p95 −96%. Seescripts/bench/updates_manifest_path_summary.md.Motivation (AI generated)
Memory sawtooth + stuck P999. Deferring manifest must not punish devices that get a bundle. Guessing is not allowed — measured under concurrent load against a selective
idx_manifest_app_version_id.Business Impact (AI generated)
Keeps delivery path healthy while making the common up-to-date check ~20× cheaper on the DB critical path.
Test Plan (AI generated)
bunx vitest run tests/plugin-pg-client-lifecycle.unit.test.tsbun run typecheck:backendbun run bench:updates-manifest-path(PASS gate: new_version not destroyed)plugin_path_timingoutcome=new_versionvs baselineLoad test evidence (AI generated)
EXPLAIN: deferred path uses Index Scan on
idx_manifest_app_version_id.Generated with AI
Summary by CodeRabbit