OUT-4104: Skip the sync job when a Dropbox webhook has no relevant changes - #143
OUT-4104: Skip the sync job when a Dropbox webhook has no relevant changes#143SandipBajracharya wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThe PR adds a Dropbox delta pre-check to avoid unnecessary sync jobs and moves webhook processing behind an immediate acknowledgement. It also introduces per-account failure isolation and pending-webhook recovery, but the delivery is still not durably owned before Dropbox receives success.
Confidence Score: 4/5The PR is not yet safe to merge because Dropbox can receive a successful acknowledgement before the notification has any durable task or recovery state. The deferred callback can be terminated before ownership is recorded, and a transient connection-query failure is swallowed before the account can be marked pending, leaving the catch-up schedule unable to recover the acknowledged change. Files Needing Attention: src/features/webhook/dropbox/api/webhook.controller.ts and src/features/webhook/dropbox/lib/webhook.service.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
D[Dropbox webhook] --> V[Validate signature]
V --> A[Return 200 and register after callback]
A --> P[Process accounts]
P --> C{Relevant changes?}
C -->|Yes or uncertain| T[Queue sync task]
C -->|No| S[Skip task]
P -->|Caught account failure| M[Mark connection pending]
M --> R[Scheduled catch-up]
A -. callback termination or pre-ID lookup failure .-> L[No task and no pending state]
Reviews (3): Last reviewed commit: "fix(OUT-4104): recover failed webhook ac..." | Re-trigger Greptile |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptileai review again |
| after(async () => { | ||
| try { | ||
| await sleep(800) // let our own writes settle first | ||
| await new DropboxWebhook().handleDropboxEvents(accounts) | ||
| } catch (error) { | ||
| // Dropbox already got its 200, so it won't retry — report so this is visible. | ||
| console.error('Dropbox webhook :: background processing failed', { accounts }, error) | ||
| Sentry.captureException(error) | ||
| } |
There was a problem hiding this comment.
When deferred account processing encounters a database, Dropbox, or Trigger.dev failure, the callback swallows the error after the route has returned 200. Because neither a sync task nor pendingWebhook is guaranteed to exist at that point, Dropbox does not retry and the affected account—plus any later accounts in the sequential loop—can remain unsynchronized.
Knowledge Base Used:
There was a problem hiding this comment.
Fixed. Each account is now processed in its own try/catch inside the loop. If it fails, we mark that connection's pendingWebhook flag so the catch-up cron re-runs it, and keep going with the other accounts — so a failure is recovered instead of silently dropped, and one bad account no longer blocks the rest. The flag is scoped to the specific connection row. Added tests for both (a failed account gets marked, and a later account still runs).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@greptileai review again |
| const dropboxWebhook = new DropboxWebhook() | ||
| await dropboxWebhook.handleDropboxEvents(accounts) | ||
| // Reply to Dropbox first, then process in the background so the check doesn't slow the reply. | ||
| after(async () => { |
There was a problem hiding this comment.
Deferred delivery lacks durability
When the after() callback is terminated before processing an account, or the active-connection lookup fails, Dropbox has already received 200 but no sync task or pending state exists. The catch-up schedule only selects connections successfully marked pending, so the acknowledged change remains unsynchronized unless another webhook later arrives.
Knowledge Base Used:
|
Closing unmerged — OUT-4104 is cancelled. Doing the change-check in the web app to skip no-op runs meant giving up Dropbox + Trigger.dev retry durability, which kept surfacing dropped-change risks; the benefit over the existing debounce was marginal. Letting the sync task do the check (its current behaviour) is simpler and durable. Nothing to revert on the mainline since this only lived on this branch. If no-op run cost proves significant (OUT-4102), revisit with a durable webhook inbox rather than a web-app pre-check. |
Dropbox fires a webhook for any change in the account, including changes outside the synced folders. Today every non-debounced webhook starts a
processDropboxChangesTrigger.dev job that lists, filters to empty, and does nothing. This adds a cheap pre-check so the job only starts when there's actually something to sync.What changed (
webhook.service.ts)handleDropboxEvents(non-debounced branch) now callstriggerIfPendingChangesinstead of always triggering.accountHasPendingChanges(account)— read-only peek: for each active channel,filesListFolderContinue(storedCursor)and short-circuittrueon the first entry (added or deleted) under the channel'sdbxRootPath(via a stack-safe recursivedeltaHasRelevantEntry). It never persists the advanced cursor — the job re-fetches from the stored one.Fail-open (never drop a real change)
Every uncertain path triggers the job: unreadable connection → trigger; channel with no cursor → trigger; missing
path_display(unmounted/edge entries) → treated as relevant → trigger; any thrown error (auth, Dropbox 409/reset) → caught and triggered. The root match is anchored (path === root || startsWith(root + '/')) so/rootdoesn't match a sibling/rootbar.Testing
New
dropbox-webhook-precheck.integration.test.ts: skip when delta is all outside root; trigger when under root; trigger on null cursor; skip when no channels; trigger on missing path_display; not match a sibling prefix; throws surfaced to the caller; and fail-open trigger throughhandleDropboxEventswhen the pre-check throws. One debounce test updated to isolate timing from the pre-check.pnpm typecheck+pnpm lintclean.Scope / follow-up
accountHasPendingChanges).lastWebhookSyncStartedAt— never runs). Trade is cheap short-circuited peeks vs. avoided Trigger.dev runs; the cron follow-up (with stamp-on-check) is where this gets fully bounded.🤖 Generated with Claude Code