fix(plugin): queue notification writes outside request path#2478
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a KV-backed plugin notification queue, a trigger POST to process queued items, a scheduled worker flush invoking the trigger, context flags and predicates to gate Supabase fallback writes and require read-replicas, local read-replica env routing, Cloudflare middleware wiring, and aligned unit/integration tests with Cloudflare gating. ChangesPlugin notification queueing system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
e08c0e3 to
492da16
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
492da16 to
2f3da4c
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
2f3da4c to
51eac6b
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
51eac6b to
c1378ed
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
c1378ed to
e0d84d8
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0d84d859b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@cloudflare_workers/api/index.ts`:
- Line 65: The imported alias plugin_notifications should be renamed to
camelCase (e.g., pluginNotifications) and all its references updated: change the
import statement importing app from
'../../supabase/functions/_backend/triggers/plugin_notifications.ts' to use
pluginNotifications, and update where it is wired into appTriggers (the
registration that still uses plugin_notifications and the route
'/plugin_notifications') to reference pluginNotifications while leaving the
route path string unchanged.
In `@cloudflare_workers/plugin/index.ts`:
- Around line 41-54: Scheduled flushes can overlap and cause duplicate
deliveries because flushQueuedPluginNotifications deletes KV only after
postPluginNotificationBatch succeeds; add a single-flight guard or server-side
dedupe. Wrap the scheduled handler invocation of flushQueuedPluginNotifications
(in scheduled() where you call scheduledApp.fetch) with a lock: obtain a
short-lived lock (preferably via a Durable Object or by setting a KV lock key
with a TTL and failing fast if it exists) before running
flushQueuedPluginNotifications, and release it after completion; alternatively
implement idempotent delivery inside
flushQueuedPluginNotifications/postPluginNotificationBatch by writing a
processed-set entry per queue key/uniqId (skip if already present) before
sending. Ensure the lock key and the processed-set use unique names and short
TTLs to avoid deadlocks and reference flushQueuedPluginNotifications,
postPluginNotificationBatch, scheduledApp and the scheduled() handler when
making changes.
In `@supabase/functions/_backend/plugins/channel_self.ts`:
- Around line 615-618: The early return inside the branch checking
canUseReadReplica && shouldSkipChannelSelfPostgresFallback(c) skips the same
device/IP rate-limit counter updates that run in the finally block of
runChannelSelfWithPgClient; move or duplicate the counter-update logic to
execute before calling logSkippedSupabaseWrite and simpleError200 so counters
are incremented for this path too. Concretely, call the same helper that updates
device/IP counters (or extract that logic from runChannelSelfWithPgClient's
finally into a shared function) and invoke it here prior to returning, while
keeping logSkippedSupabaseWrite and simpleError200 as-is.
In `@supabase/functions/_backend/utils/notifications.ts`:
- Around line 275-276: The fallback branch creates a new PG client via
getPgClient(c) wrapped by createDrizzleClient but never closes it, leaking
connections; update the code around effectiveWriteClient and the call to
insertNotificationClaim so that when writeClient is undefined you create the
client, call insertNotificationClaim inside a try block, and always close the
internally-created PG client in finally (i.e., close the pool returned by
getPgClient or call the appropriate teardown on the created drizzle client);
apply the same try/finally pattern to the second occurrence around the other
effectiveWriteClient/createDrizzleClient use at the later site.
In `@supabase/functions/_backend/utils/plugin_notification_flush.ts`:
- Around line 54-127: SonarCloud flags flushQueuedPluginNotifications for high
cognitive complexity; extract the inner batch processing (the block that handles
items array: calling postPluginNotificationBatch, deleting keys on success,
incrementing transferred/failed counts, and catching/logging errors) into a new
helper function processBatch(c: Context, store: KVNamespace, items:
PluginNotificationQueueItem[], itemKeys: string[]): Promise<{
transferred:number; deleted:number; failed:number }>, and call it from the loop
in flushQueuedPluginNotifications; keep existing calls to
parsePluginNotificationQueueItem, postPluginNotificationBatch and
cloudlogErr/cloudlog but move the try/catch and counter updates into
processBatch so the main loop only handles listing, reading, parsing, pushing
items/itemKeys, and aggregating results from processBatch.
In `@supabase/functions/_backend/utils/plugin_notification_queue.ts`:
- Around line 90-110: queuePluginOrgMembersNotification currently takes 8 scalar
parameters (c, eventName, preferenceKey, eventData, orgId, uniqId, cron,
audience), which exceeds the recommended parameter count; refactor its signature
to accept an options object as the second parameter (e.g., options: { eventName,
preferenceKey, eventData, orgId, uniqId, cron, audience }) and update the
function body to read those properties (and pass them into
enqueuePluginNotification either by spreading options or by explicit fields) to
reduce parameter count; note you must also update all call sites to construct
and pass the new options object or defer this change if broader refactoring
across notification helpers is not feasible now.
- Around line 60-63: Remove the unnecessary type assertion on payload: when
constructing const payload = { ...item, enqueuedAt: new Date().toISOString() },
drop the trailing "as PluginNotificationQueueItem" since TypeScript can infer
the discriminated union from item (which already contains type: 'org' |
'org_members') and the added enqueuedAt field; update the payload declaration to
use the plain object and ensure the compiler still type-checks in the
surrounding code that uses payload (references: payload, item, enqueuedAt,
PluginNotificationQueueItem).
In `@supabase/functions/_backend/utils/supabase_write_guard.ts`:
- Around line 15-22: The getBooleanContextFlag function uses an explicit type
assertion on the Context parameter which is unnecessary; remove the long cast
"(c as Context & { get: (key: string) => unknown })" and call c.get(...)
directly (or cast the key to any) inside the existing try/catch so the runtime
behavior stays the same and TypeScript is satisfied; update the function body to
use c.get(key) === true while keeping the catch branch returning false.
In `@tests/stats-download.test.ts`:
- Line 12: The comment above the USE_CLOUDFLARE flag in
tests/stats-download.test.ts is inaccurate; replace or remove the line
referencing "serialize paths for consistency" so it correctly states that
USE_CLOUDFLARE is used to conditionally skip tests that expect primary database
writes (or simply note it toggles Cloudflare-worker behavior), and ensure the
comment directly references the USE_CLOUDFLARE flag so future readers understand
its purpose when reading the test file.
🪄 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: d1a6fe45-caac-4a5a-a4ab-8653bb41252c
📒 Files selected for processing (22)
cloudflare_workers/api/index.tscloudflare_workers/plugin/index.tscloudflare_workers/plugin/wrangler.jsoncscripts/start-cloudflare-workers.shsupabase/functions/_backend/plugins/channel_self.tssupabase/functions/_backend/plugins/stats.tssupabase/functions/_backend/triggers/plugin_notifications.tssupabase/functions/_backend/utils/cloudflare.tssupabase/functions/_backend/utils/hono.tssupabase/functions/_backend/utils/notifications.tssupabase/functions/_backend/utils/org_email_notifications.tssupabase/functions/_backend/utils/pg.tssupabase/functions/_backend/utils/plugin_notification_flush.tssupabase/functions/_backend/utils/plugin_notification_queue.tssupabase/functions/_backend/utils/stats.tssupabase/functions/_backend/utils/supabase_write_guard.tstests/channel_self.test.tstests/key_id_e2e.test.tstests/plugin-supabase-write-guard.unit.test.tstests/stats-download.test.tstests/stats.test.tstests/updates.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6902d91a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 726d83be83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9ebe16d86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jinhongliang991013
left a comment
jinhongliang991013
left a comment
There was a problem hiding this comment.
Reviewed the current queue/trigger path after the previous reliability fixes. One remaining delivery gap affects org-member notifications with more than one eligible recipient.
…tion-kv-queue # Conflicts: # cloudflare_workers/api/wrangler.jsonc # supabase/functions/_backend/plugins/stats.ts # supabase/functions/_backend/triggers/logsnag_insights.ts # supabase/functions/_backend/utils/update.ts
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_45fa35cb-58f8-456d-a9ba-29e9bb21103e) |
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_1e406ea4-e212-496a-b7e4-1333ceeecf2a) |
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_e88c2d37-c087-4e7b-89bd-e127f364750c) |
…tion-kv-queue # Conflicts: # supabase/functions/_backend/utils/stats.ts
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_c7a0f435-e95e-4414-a172-4b715ffd37bc) |
|
2 participants
Summary (AI generated)
plugin_notificationstrigger to process transferred items through the existing notification helpers outside the hot plugin path.Motivation (AI generated)
Plugin endpoints are hot public device endpoints and should not read or write the primary Supabase database for notification side effects. Queuing notification work in KV lets plugin requests stay replica/KV/Analytics-only while preserving the email workflow through a controlled background trigger.
Business Impact (AI generated)
This reduces primary database pressure and tail latency risk on
/updates,/stats, and/channel_self, while keeping billing/plugin/channel-self alert emails flowing through a delayed background path. It also makes missing Worker bindings fail closed instead of silently writing to Supabase from plugin traffic.Test Plan (AI generated)
bunx vitest run tests/plugin-supabase-write-guard.unit.test.tsbun typecheckbun lint:backendbun lint(passes with existingsrc/services/compatibilityEvents.tswarning)bunx wrangler deploy --dry-run --config cloudflare_workers/plugin/wrangler.jsonc --env prod_eubunx wrangler deploy --dry-run --config cloudflare_workers/api/wrangler.jsonc --env prodGenerated with AI
Summary by CodeRabbit
New Features
Infrastructure
Bug Fixes
Tests
Note
High Risk
Changes behavior on hot public plugin endpoints (async email delivery, no primary stats/device writes on Cloudflare, stricter channel_self storage), plus minute-level cron and notification claim rollback—incorrect config or flush failures can drop or duplicate alerts.
Overview
Plugin Worker requests no longer perform primary-database side effects for stats, notification claims, or legacy
channel_selfPostgres storage. Middleware sets context flags that skip Supabase fallbacks, require read replicas (with optionalLOCAL_READ_REPLICA_SUPABASE_DB_URL), and enqueue org email work to a newPLUGIN_NOTIFICATION_QUEUEKV binding instead of calling Bento during the request.Background delivery adds a
plugin_notificationstrigger (API + Supabase triggers router) that processes batched queue items through existingsendNotifOrg/sendNotifToOrgMembersOncehelpers, plusflushQueuedPluginNotificationswith per-item locks, throttle markers, and retry on failure. The API worker runs a* * * * *cron that calls an internalflush-plugin-notificationsroute and fails the scheduled run if any transfers fail.Notification correctness changes include rolling back notification DB claims when Bento send fails, closing owned PG clients in send paths, and treating an existing org-level claim in
sendNotifToOrgMembersOnceas already delivered.channel_selfawaits IP rate-limit recording safely and returnschannel_self_server_storage_unavailablewhen Postgres fallback is blocked and KV/replica routing is insufficient.Tests and docs: integration tests split between Cloudflare (no primary writes / KV
channel_self) vs Supabase paths; new unit tests cover queue flush, trigger batching, and write-guard policy.cli/README.mdis expanded with marketing, quick start, and CI examples (unrelated to runtime behavior).Reviewed by Cursor Bugbot for commit e25f48c. Bugbot is set up for automated code reviews on this repo. Configure here.