feat: Plan 7 PR A — long-lived SSE edge sync - #15
Conversation
Rewrite GET /v1/sync/events as a poll-driven SSE session with Decision 15 cursor parsing, snapshot-consistent backlog probe (429), bounded write queue + shutdown control lane, heartbeats, snapshot-only compression, and graceful SSE drain on SIGTERM. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughChangesSSE edge sync
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncRoute
participant SseRegistry
participant syncRepository
participant runSseSession
participant SseWriteQueue
Client->>SyncRoute: request /events with cursor
SyncRoute->>SseRegistry: check admission
SyncRoute->>syncRepository: load initial batch and high-water
SyncRoute->>runSseSession: start SSE session
runSseSession->>SseWriteQueue: enqueue initial events and cursor
runSseSession->>syncRepository: poll for new events
syncRepository-->>runSseSession: return events and high-water
runSseSession->>SseWriteQueue: enqueue events and heartbeat frames
SseWriteQueue-->>Client: stream SSE frames
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1.
|
PR Summary by Qodofeat: long-lived SSE sync stream with backpressure, backlog cap, graceful drain
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Review: PR #15 — Plan 7 PR A implementation
Overall
High-quality implementation that faithfully follows the plan doc. The architecture (write queue with control lane, snapshot-consistent DB reads, registry for admission/shutdown) is well-thought-out. Tests exist for the new units.
Issues requiring changes
1. getInitialBatchWithHighWater — tenant filtering regression (syncRepository.ts:90-94)
When tenantId is undefined, the SELECT adds AND tenant_id IS NULL. This means non-tenant requests only return NULL-tenant events, excluding all tenant-scoped events. The old getEventsSince returned ALL events when tenantId was undefined. This is a behavior regression that could cause data loss.
Fix: only add AND (tenant_id IS NULL OR ...) when tenantId is provided. When undefined, return all events (no tenant filter).
2. Same bug in getPollBatchWithHighWater (syncRepository.ts:143-146)
Same issue: when tenantId is undefined, the query incorrectly adds AND tenant_id IS NULL.
Recommendations (non-blocking)
SseWriteQueue.pump()usesqueueMicrotask— on high throughput this could starve the event loop. ConsidersetImmediateor a threshold-based scheduler.- The
sendProblemhttps://verilink.dev/problems/...URL doesnt exist yet — fine for RFC 9457 compliance but worth documenting. - No integration test in this PR — acceptable per the PR split plan.
REPEATABLE READisolation is correct but may need a retry loop under contention (deferrable).
Verdict
Two tenant-filtering correctness bugs that should be fixed. Everything else is solid.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@control-plane/src/domains/sync/sseSession.ts`:
- Around line 160-196: In the SSE session polling loop around
getPollBatchWithHighWater, add monitoring for per-connection polling load and
database latency, including connection/session context where available. Preserve
the existing poll-driven behavior and event delivery logic; use the metrics or
observability conventions already established in the surrounding module.
In `@control-plane/src/domains/sync/sseWriteQueue.ts`:
- Around line 62-68: Update the frame classification logic in the queue’s
enqueue/write method to return “oversized” whenever the frame size exceeds
either maxFrameBytes or maxBytes, before the “full” capacity check. Preserve
“full” only for frames that can fit the byte budget but cannot currently be
queued, so waitForSpace(size) can eventually succeed.
In `@control-plane/src/domains/sync/syncRepository.ts`:
- Around line 85-176: Extract the duplicated snapshot transaction,
tenant-filtered event query, high-water retrieval, commit/rollback, and client
release logic from getInitialBatchWithHighWater and getPollBatchWithHighWater
into a shared fetchEventsAndHighWaterInSnapshot helper. Keep
getInitialBatchWithHighWater responsible for requesting maxBatch + 1 events,
truncating the result, and setting backlogExceeded; have
getPollBatchWithHighWater delegate with limit unchanged.
- Around line 85-131: Update getInitialBatchWithHighWater and the SyncEvent
definition so BIGINT sync_version values use one consistent runtime type: either
convert query results to the existing number contract before returning events,
or change SyncEvent and all consumers to string/bigint. Ensure highWater and
event sync_version handling follow the same representation without unsafe
implicit assumptions.
In `@control-plane/src/index.ts`:
- Around line 40-49: Update the watchdog timeout in the shutdown flow around
forceExit to derive its duration from config.syncSse.shutdownDrainMs rather than
the hardcoded 10000ms value, preserving the existing forced-exit behavior while
allowing the configured SSE drain period to complete.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 226bfe64-1aef-47b0-9ff4-721dba8ff5c9
⛔ Files ignored due to path filters (1)
control-plane/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
control-plane/package.jsoncontrol-plane/src/config.tscontrol-plane/src/domains/sync/parseLastEventId.test.tscontrol-plane/src/domains/sync/parseLastEventId.tscontrol-plane/src/domains/sync/sseRegistry.test.tscontrol-plane/src/domains/sync/sseRegistry.tscontrol-plane/src/domains/sync/sseSession.tscontrol-plane/src/domains/sync/sseWriteQueue.test.tscontrol-plane/src/domains/sync/sseWriteQueue.tscontrol-plane/src/domains/sync/syncRepository.tscontrol-plane/src/domains/sync/syncService.tscontrol-plane/src/index.tscontrol-plane/src/routes/sync.tscontrol-plane/src/shared/errors/AppError.tsdocs/superpowers/plans/HANDOVER.md
Fix sleep abort listener leak, write-queue backpressure/drain guards, oversized vs full byte budget, Postgres BIGINT Last-Event-ID bounds, tenant-unscoped event reads, shared snapshot fetch helper, sync_version typing, poll latency debug logs, and shutdown watchdog vs drainMs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Review of latest commit (08d6b0a)
All previous concerns are addressed:
1. Tenant filtering regression — FIXED — fetchEventsAndHighWaterInSnapshot only adds tenant filter when tenantId is provided. The old else { AND tenant_id IS NULL } bug is removed from both snapshot queries and getEventsSince.
2. PG_BIGINT_MAX guard — ADDED — rejects values above 9223372036854775807n with test coverage.
3. inflightBytes tracking — ADDED — SseWriteQueue now tracks bytes written to socket but not yet drained for accurate backpressure.
4. Socket error handling — ADDED — close/error listeners on response, safeWrite wrapper.
5. Sleep abort listener leak — FIXED — cleanup() runs on both resolve and reject with proper removeEventListener.
6. Shared DB helper — EXTRACTED — fetchEventsAndHighWaterInSnapshot eliminates duplication.
7. Oversized byte budget — FIXED — enqueueData now checks size > opts.maxBytes alongside maxFrameBytes.
Minor observation (non-blocking)
sleep() uses addEventListener without { once: true }. The cleanup function calls removeEventListener on resolve, so it is not a leak — but if abort fires between addEventListener and the timeout, the listener stays registered (harmlessly, since the signal is already aborted). Minor.
Verdict
All review feedback addressed. Clean commit. Ready to ship.
Summary
GET /v1/sync/eventsSSE: heartbeats, poll loop, bootstrap cursor, bounded write queue (frames + bytes) withevent: shutdowncontrol lane429+X-Verilink-Sync-Reason: backlog-cap(not410); strict raw-stringLast-Event-IDparsingTest plan
cd control-plane && npm run test:unitcd control-plane && npx tsc --noEmit: pingand cursor events@coderabbitai review
Summary by CodeRabbit
New Features
Bug Fixes
Tests