ENG-298 repeat the sync at regular intervals #513
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughRefactored the Supabase sync module to introduce a centralized, time-based scheduler with persistent state management. Added exponential backoff on failures with hard caps, introduced FatalError handling for unrecoverable conditions, updated type signatures with Date objects, and exposed new control functions (setSyncActivity, initializeSupabaseSync) to manage sync lifecycle and conditional toast notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant initializeSupabaseSync
participant Scheduler
participant createOrUpdateDiscourseEmbedding
participant Supabase
participant FatalError
rect rgb(230, 245, 245)
Note over initializeSupabaseSync: Initialization Phase
Caller->>initializeSupabaseSync: initializeSupabaseSync()
initializeSupabaseSync->>initializeSupabaseSync: Check createClient() result
alt Client valid
initializeSupabaseSync->>Scheduler: Schedule first run via activeTimeout
Scheduler-->>initializeSupabaseSync: activeTimeout set
else Client invalid
initializeSupabaseSync->>FatalError: FatalError raised
FatalError-->>initializeSupabaseSync: Disable syncing
end
end
rect rgb(245, 240, 230)
Note over Scheduler: Scheduler Phase (Time-based Loop)
loop Every aBASE_SYNC_INTERVAL ms
Scheduler->>createOrUpdateDiscourseEmbedding: Execute sync
alt doSync enabled & Client valid
createOrUpdateDiscourseEmbedding->>Supabase: proposeSyncTask()
Supabase-->>createOrUpdateDiscourseEmbedding: SyncTaskInfo {lastUpdateTime?, nextUpdateTime?}
alt Task needs update
createOrUpdateDiscourseEmbedding->>Supabase: Perform sync
Supabase-->>createOrUpdateDiscourseEmbedding: Success
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: Reset numFailures=0
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: endSyncTask(showToast=true)
else Task update failed
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: numFailures++
alt numFailures < MAX_FAILURES
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: Exponential backoff
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: endSyncTask(showToast=true)
else numFailures >= MAX_FAILURES
createOrUpdateDiscourseEmbedding->>FatalError: FatalError raised
FatalError-->>createOrUpdateDiscourseEmbedding: Stop scheduling
end
end
else doSync disabled or setup error
createOrUpdateDiscourseEmbedding->>createOrUpdateDiscourseEmbedding: Cancel activeTimeout
end
end
end
rect rgb(240, 245, 230)
Note over Caller: Manual Control
Caller->>Caller: setSyncActivity(false)
Caller->>Scheduler: Cancel activeTimeout
Caller->>Caller: setSyncActivity(true)
Caller->>Scheduler: Resume scheduling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: The changes concentrate significant logic restructuring into a single file, introducing persistent scheduler patterns with state management (activeTimeout, numFailures), exponential backoff strategy, FatalError handling for lifecycle control, and multiple interdependent exported functions. While file scope is narrow, the control-flow density, new scheduler semantics, and error handling pathways require careful review to verify correctness of state transitions, timeout cleanup, and failure recovery logic. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
327-329: Bug: Error constructor misused; second argument is ignored (should beoptions.cause).Use
causeto preserve the original error and include its message.-throw new Error(`upsert_content failed for batch ${idx + 1}:`, error); +throw new Error( + `upsert_content failed for batch ${idx + 1}: ${'message' in (error ?? {}) ? (error as any).message : String(error)}`, + { cause: error } +);
🧹 Nitpick comments (3)
apps/roam/src/utils/syncDgNodesToSupabase.ts (3)
415-426: Schedule clamp: guard against negative or absurd delays.Clock drift or rounding can yield negative/huge delays. Clamp to a sane minimum and optional max.
- activeTimeout = setTimeout( - createOrUpdateDiscourseEmbedding, // eslint-disable-line @typescript-eslint/no-misused-promises - nextUpdateTime.valueOf() - Date.now() + 100, - ); + const rawDelay = nextUpdateTime.valueOf() - Date.now() + 100; + const delay = Math.max(100, Math.min(rawDelay, 30 * 60 * 1000)); // [100ms, 30m] + activeTimeout = setTimeout( + createOrUpdateDiscourseEmbedding, // eslint-disable-line @typescript-eslint/no-misused-promises + delay, + );
475-485: Backoff lacks jitter and cap; add jitter to avoid thundering herd and cap extreme delays.Introduce jitter and a max backoff. Also, confirm partial-success policy per team learning.
- let timeout = BASE_SYNC_INTERVAL; + let timeout = BASE_SYNC_INTERVAL; if (success) { numFailures = 0; } else { numFailures += 1; if (numFailures >= MAX_FAILURES) { doSync = false; return; } - timeout *= 2 ** numFailures; + // Exponential backoff with jitter and cap + const maxBackoff = 30 * 60 * 1000; // 30 minutes + const exp = timeout * (2 ** numFailures); + const jitter = 0.9 + Math.random() * 0.2; // 0.9x–1.1x + timeout = Math.min(exp, maxBackoff) * jitter; }Based on learnings: confirm that “partial successes should not trigger backoff.” Does any upstream call here return counts vs throwing? If counts can indicate partial progress, ensure
successremains true and don’t enter backoff.
435-439: Minor: avoid creating and discarding a client instance.createClient() is used as a truthy check but constructs a client you immediately drop. Prefer a dedicated “isConfigured” helper or reuse the instance.
- if (!createClient()) { + const bootstrap = createClient(); + if (!bootstrap) { // not worth retrying // TODO: Differentiate setup vs connetion error throw new FatalError("Could not access supabase."); }Optionally plumb
bootstrapthrough when you need a non-authenticated client.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/syncDgNodesToSupabase.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
🔇 Additional comments (3)
apps/roam/src/utils/syncDgNodesToSupabase.ts (3)
64-73: Toast gating looks good.showToast is respected on both error paths; LGTM.
Also applies to: 91-97
392-405: Activation toggling is clean and race-safe enough for this scope.Clears pending timeout on pause; schedules a quick run on resume. LGTM.
504-509: Init path: good first-run UX.Conditional enable + initial toast run looks good.
maparent
changed the title
First not sure how I reached to this state but is it possible to have |
| } from "./conceptConversion"; | ||
| import { fetchEmbeddingsForNodes } from "./upsertNodesAsContentWithEmbeddings"; | ||
| import { convertRoamNodeToLocalContent } from "./upsertNodesAsContentWithEmbeddings"; | ||
| import { getRoamUrl } from "roamjs-components/dom"; |
| doSync = false; | ||
| return; | ||
| } | ||
| const jitter = 0.9 + Math.random() * 0.2; // 0.9x–1.1x |
There was a problem hiding this comment.
@maparent I am curious about this as well
|
Thank you. Will merge just after #511 is merged. |
Summary by CodeRabbit