ENG-976: Repair sync function #511
Conversation
|
@CodeRabbit review |
|
Updates to Preview Branch (ENG-976-repair-propose-sync) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new PL/pgSQL function Changes
Sequence DiagramsequenceDiagram
participant Caller
participant propose_sync_task
participant sync_info DB
participant Advisory Lock
Caller->>propose_sync_task: propose_sync_task(target, function, worker, timeout, interval)
propose_sync_task->>sync_info DB: Attempt INSERT new sync_info row
alt Row inserted (first time)
sync_info DB-->>propose_sync_task: Success
propose_sync_task-->>Caller: Return last_run timestamp
else Row exists
propose_sync_task->>Advisory Lock: Acquire lock on row
Advisory Lock-->>propose_sync_task: Lock acquired
propose_sync_task->>sync_info DB: Load status, failure_count, timing fields
sync_info DB-->>propose_sync_task: Current state
propose_sync_task->>propose_sync_task: Apply backoff logic<br/>Check timeout & timing constraints
alt Task ready to execute
propose_sync_task->>sync_info DB: Query last successful run time
sync_info DB-->>propose_sync_task: max(last_task_start)
propose_sync_task->>sync_info DB: UPDATE row (active, deadlines, worker)
sync_info DB-->>propose_sync_task: Updated
else Task not ready
propose_sync_task->>propose_sync_task: Compute next eligible time
end
propose_sync_task->>Advisory Lock: Release lock
Advisory Lock-->>propose_sync_task: Released
propose_sync_task-->>Caller: Return timestamp (run or next eligible time)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes The changes introduce moderately dense PL/pgSQL logic involving state management, advisory locking mechanisms, backoff calculations, and conditional timing logic. The function coordinates multiple database operations with constraints and decision branches that require careful verification of correctness, especially around lock handling, race conditions, and the timing/backoff calculations. Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/database/supabase/migrations/20251023130549_repair_propose_sync.sql (1)
52-66: Return value semantics are contextual; document for callers.The function returns either:
- The last successful task's
last_task_starttimestamp (when ready to execute), or- The next eligible execution time (when not ready).
Callers need to understand this dual semantics. Additionally,
NULLcan be returned (when status='complete' never occurred), which represents "never successfully completed," distinct from "not yet ready."Consider documenting the return value contract in a function comment for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/database/supabase/migrations/20251023130549_repair_propose_sync.sql(1 hunks)packages/database/supabase/schemas/sync.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Supabase Preview
🔇 Additional comments (2)
packages/database/supabase/schemas/sync.sql (1)
95-174: Logic for capturing last successful task timestamp is sound.The addition at lines 146–149 correctly captures the last successful task's
last_task_starttimestamp before overwriting it with the current invocation'sstart_timeduring the UPDATE. This ensures the return value reflects the previously completed task time, as intended by the PR objective.The logic is protected by advisory lock (line 132–163), so the SELECT–UPDATE sequence is atomic within the lock scope. The unique index on
(sync_target, sync_function)guarantees at most one row per task, so themax()aggregation is deterministic.packages/database/supabase/migrations/20251023130549_repair_propose_sync.sql (1)
1-72: Function design and concurrency handling are solid.The migration correctly implements the repair. Key strengths:
- Advisory lock pattern (lines 38, 69): Properly brackets all state mutations, ensuring atomic transitions under lock.
- INSERT idempotence (line 25):
ON CONFLICT DO NOTHINGprevents duplicate row creation; caller tolerates seeing an already-created row.- Backoff strategy (line 49): Multiplies interval by
(1 + failure_count), creating an exponential-style backoff without explicit exponential calculation.- Timeout detection (line 44): Correctly identifies when an active task has exceeded its deadline.
- Precondition assertions (lines 20–22): Validate that
timeout * 2 < task_intervalto ensure a safety margin exists.The critical fix—lines 52–55—captures
last_task_startbefore the UPDATE overwrites it, ensuring the caller receives the last successful task's timestamp rather than the current invocation's start time.
This repairs the sync function so it actually gives the latest complete task.
Summary by CodeRabbit