-
Notifications
You must be signed in to change notification settings - Fork 0
OUT-4104: Skip the sync job when a Dropbox webhook has no relevant changes #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
adad40b
60fd73c
7686c81
f171d05
7797cf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import crypto from 'node:crypto' | ||
| import * as Sentry from '@sentry/nextjs' | ||
| import status from 'http-status' | ||
| import { type NextRequest, NextResponse } from 'next/server' | ||
| import { after, type NextRequest, NextResponse } from 'next/server' | ||
| import env from '@/config/server.env' | ||
| import { DropboxWebhook } from '@/features/webhook/dropbox/lib/webhook.service' | ||
| import { sleep } from '@/utils/sleep' | ||
|
|
@@ -31,8 +32,6 @@ export const handleWebhookEvents = async (req: NextRequest) => { | |
|
|
||
| const body = await req.text() | ||
|
|
||
| await sleep(800) // prevent ping-pong case of webhooks | ||
|
|
||
| const computedSignature = crypto | ||
| .createHmac('sha256', env.DROPBOX_APP_SECRET) | ||
| .update(body) | ||
|
|
@@ -48,8 +47,17 @@ export const handleWebhookEvents = async (req: NextRequest) => { | |
| const { list_folder } = JSON.parse(body) | ||
| const accounts = list_folder?.accounts ?? [] | ||
|
|
||
| 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 () => { | ||
| 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) | ||
| } | ||
|
Comment on lines
+51
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Knowledge Base Used:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Each account is now processed in its own try/catch inside the loop. If it fails, we mark that connection's |
||
| }) | ||
|
|
||
| // Dropbox expects a 200 OK with plain text body | ||
| return new NextResponse('', { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { DropboxWebhook } from '@/features/webhook/dropbox/lib/webhook.service' | ||
| import { processDropboxChanges } from '@/trigger/processFileSync' | ||
| import { dropboxEntryFactory } from '../factories' | ||
| import { mockDropboxRpc, paginateDropboxListFolder, server } from '../msw' | ||
| import { channelSeeder, dropboxConnectionSeeder } from '../seeders' | ||
|
|
||
| const seedAccount = () => | ||
| dropboxConnectionSeeder.create({ accountId: 'acc', rootNamespaceId: 'ns', refreshToken: 'rt' }) | ||
|
|
||
| afterEach(() => vi.restoreAllMocks()) | ||
|
|
||
| describe('DropboxWebhook#accountHasPendingChanges', () => { | ||
| it('returns false when no delta entry falls under a channel root', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: 'cursor:0', | ||
| }) | ||
| // The whole delta is outside the synced root — nothing to sync. | ||
| server.use( | ||
| ...paginateDropboxListFolder([dropboxEntryFactory.build({ path_display: '/other/x.txt' })]), | ||
| ) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(false) | ||
| }) | ||
|
|
||
| it('returns true when a delta entry falls under a channel root', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: 'cursor:0', | ||
| }) | ||
| server.use( | ||
| ...paginateDropboxListFolder([dropboxEntryFactory.build({ path_display: '/root/x.txt' })]), | ||
| ) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(true) | ||
| }) | ||
|
|
||
| it('returns true when a channel has no cursor (cannot peek safely)', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: null, | ||
| }) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false when the account has no active channels', async () => { | ||
| await seedAccount() | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(false) | ||
| }) | ||
|
|
||
| it('returns false when the connection has no refresh token (job cannot sync)', async () => { | ||
| await dropboxConnectionSeeder.create({ | ||
| accountId: 'acc', | ||
| rootNamespaceId: 'ns', | ||
| refreshToken: null, | ||
| }) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(false) | ||
| }) | ||
|
|
||
| it('throws on a Dropbox error so the caller can fail open and trigger', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: 'cursor:0', | ||
| }) | ||
| mockDropboxRpc('/2/files/list_folder/continue', () => | ||
| Response.json({ error_summary: 'reset/', error: { '.tag': 'reset' } }, { status: 409 }), | ||
| ) | ||
|
|
||
| await expect(new DropboxWebhook().accountHasPendingChanges('acc')).rejects.toBeDefined() | ||
| }) | ||
|
|
||
| it('returns true when an entry has no path_display (fail open)', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: 'cursor:0', | ||
| }) | ||
| // Raw entry with no path_display (e.g. unmounted) — can't tell → treat as relevant. | ||
| server.use(...paginateDropboxListFolder([{ '.tag': 'deleted', name: 'x' }])) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(true) | ||
| }) | ||
|
|
||
| it('does not match a sibling folder that shares the root prefix', async () => { | ||
| const connection = await seedAccount() | ||
| await channelSeeder.create({ | ||
| portalId: connection.portalId, | ||
| dbxRootPath: '/root', | ||
| dbxCursor: 'cursor:0', | ||
| }) | ||
| server.use( | ||
| ...paginateDropboxListFolder([dropboxEntryFactory.build({ path_display: '/rootbar/x.txt' })]), | ||
| ) | ||
|
|
||
| expect(await new DropboxWebhook().accountHasPendingChanges('acc')).toBe(false) | ||
| }) | ||
|
|
||
| it('fails open: handleDropboxEvents triggers the job when the pre-check throws', async () => { | ||
| await seedAccount() // lastWebhookSyncStartedAt null → not debounced → reaches the pre-check | ||
| vi.spyOn(DropboxWebhook.prototype, 'accountHasPendingChanges').mockRejectedValue( | ||
| new Error('boom'), | ||
| ) | ||
| const triggerSpy = vi | ||
| .spyOn(processDropboxChanges, 'trigger') | ||
| .mockResolvedValue(undefined as never) | ||
|
|
||
| await new DropboxWebhook().handleDropboxEvents(['acc']) | ||
|
|
||
| expect(triggerSpy).toHaveBeenCalledWith('acc', { concurrencyKey: 'acc' }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: