Skip to content

fix(knowledge): replace deletion-safety heuristics with a two-phase tombstone #5884

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.

Sign up for GitHub

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

Open
waleedlatif1 wants to merge 1 commit into
base: staging
Choose a base branch
from
+161 −207
Open

fix(knowledge): replace deletion-safety heuristics with a two-phase tombstone #5884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 4 additions & 33 deletions apps/sim/connectors/google-sheets/google-sheets.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,40 +116,26 @@ describe('googleSheetsConnector trashed handling', () => {
})

describe('listDocuments', () => {
it('returns an empty listing and confirms the empty result when the spreadsheet is trashed', async () => {
it('returns an empty listing when the spreadsheet is trashed', async () => {
stubFetch({
drive: { status: 200, body: { trashed: true, modifiedTime: '2026-07-01T00:00:00.000Z' } },
})
const syncContext: Record<string, unknown> = {}

const result = await googleSheetsConnector.listDocuments(
ACCESS_TOKEN,
SOURCE_CONFIG,
undefined,
syncContext
)
const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG)

expect(result).toEqual({ documents: [], hasMore: false })
expect(syncContext.sourceConfirmedEmpty).toBe(true)
})

it('lists every tab when the trashed field is absent', async () => {
stubFetch({ drive: { status: 200, body: { modifiedTime: '2026-07-01T00:00:00.000Z' } } })
const syncContext: Record<string, unknown> = {}

const result = await googleSheetsConnector.listDocuments(
ACCESS_TOKEN,
SOURCE_CONFIG,
undefined,
syncContext
)
const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG)

expect(result.documents.map((d) => d.externalId)).toEqual([
`${SPREADSHEET_ID}__sheet__0`,
`${SPREADSHEET_ID}__sheet__7`,
])
expect(result.hasMore).toBe(false)
expect(syncContext.sourceConfirmedEmpty).toBeUndefined()
})

it('lists every tab when trashed is explicitly false', async () => {
Expand All @@ -162,20 +148,13 @@ describe('googleSheetsConnector trashed handling', () => {

it('fails open and lists every tab when the Drive read fails', async () => {
stubFetch({ drive: { status: 500, body: { error: 'backend error' } } })
const syncContext: Record<string, unknown> = {}

const result = await googleSheetsConnector.listDocuments(
ACCESS_TOKEN,
SOURCE_CONFIG,
undefined,
syncContext
)
const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG)

expect(result.documents.map((d) => d.externalId)).toEqual([
`${SPREADSHEET_ID}__sheet__0`,
`${SPREADSHEET_ID}__sheet__7`,
])
expect(syncContext.sourceConfirmedEmpty).toBeUndefined()
})

it('fails open when the Drive body is not an object', async () => {
Expand All @@ -185,14 +164,6 @@ describe('googleSheetsConnector trashed handling', () => {

expect(result.documents).toHaveLength(2)
})

it('does not throw when trashed and no syncContext is passed', async () => {
stubFetch({ drive: { status: 200, body: { trashed: true } } })

const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG)

expect(result).toEqual({ documents: [], hasMore: false })
})
})

describe('getDocument', () => {
Expand Down
16 changes: 6 additions & 10 deletions apps/sim/connectors/google-sheets/google-sheets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ export const googleSheetsConnector: ConnectorConfig = {
accessToken: string,
sourceConfig: Record<string, unknown>,
_cursor?: string,
syncContext?: Record<string, unknown>
_syncContext?: Record<string, unknown>
): Promise<ExternalDocumentList> => {
const spreadsheetId = (sourceConfig.spreadsheetId as string)?.trim()
if (!spreadsheetId) {
Expand All @@ -269,18 +269,14 @@ export const googleSheetsConnector: ConnectorConfig = {

/**
* A trashed spreadsheet is no longer current content, so it drops out of the
* listing and stops being re-indexed. Unlike an ordinary empty listing page
* (which could equally mean the source is unreachable), this is a direct,
* single-resource confirmation from the Drive API that the spreadsheet itself
* is gone — so `sourceConfirmedEmpty` tells the sync engine's zero-document
* guard it's safe to reconcile (purge the stored tabs) on this sync, rather
* than requiring a forced full resync. `validateConfig` reports the trashed
* state so the connector does not look healthy while serving tabs from a file
* its owner has thrown away.
* listing and stops being re-indexed. The sync engine reconciles its absence
* the same way it does for every connector: pending-removal on the first
* sync that doesn't see it, purged once a later sync confirms it's still
* gone. `validateConfig` reports the trashed state so the connector does not
* look healthy while serving tabs from a file its owner has thrown away.
*/
if (isTrashedDriveFile(driveMetadata)) {
logger.info('Spreadsheet is in the Drive trash; listing no documents', { spreadsheetId })
if (syncContext) syncContext.sourceConfirmedEmpty = true
return { documents: [], hasMore: false }
}

Expand Down
114 changes: 51 additions & 63 deletions apps/sim/lib/knowledge/connectors/sync-engine.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,98 +76,86 @@ describe('shouldReconcileDeletions', () => {
})
})

describe('shouldSkipEmptyListing', () => {
it('does not skip when the listing is non-empty', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')
describe('partitionSyncReconciliation', () => {
const live = (id: string, externalId: string | null = id) => ({ id, externalId })

expect(shouldSkipEmptyListing(1, 5, undefined, {})).toBe(false)
})
it('marks a live document missing from the listing as pending removal, not hard-deleted', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

it('does not skip when there are no existing documents to lose', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')
const result = partitionSyncReconciliation([live('a')], [], new Set(), undefined)

expect(shouldSkipEmptyListing(0, 0, undefined, {})).toBe(false)
expect(result).toEqual({ resurrectIds: [], softDeleteIds: ['a'], hardDeleteIds: [] })
})

it('does not skip on a forced fullSync', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')

expect(shouldSkipEmptyListing(0, 5, true, {})).toBe(false)
})
it('hard-deletes a document already pending removal that is still absent', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

it('skips by default on an empty listing with existing documents', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')
const result = partitionSyncReconciliation([], [live('a')], new Set(), undefined)

expect(shouldSkipEmptyListing(0, 5, undefined, {})).toBe(true)
expect(shouldSkipEmptyListing(0, 5, undefined, undefined)).toBe(true)
expect(shouldSkipEmptyListing(0, 5, false, {})).toBe(true)
expect(result).toEqual({ resurrectIds: [], softDeleteIds: [], hardDeleteIds: ['a'] })
})

it('does not skip when the connector confirms the empty result against the source', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')

expect(shouldSkipEmptyListing(0, 5, undefined, { sourceConfirmedEmpty: true })).toBe(false)
})
it('resurrects a pending-removal document that reappears in the listing', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

it('still skips when sourceConfirmedEmpty is falsy', async () => {
const { shouldSkipEmptyListing } = await import('@/lib/knowledge/connectors/sync-engine')
const result = partitionSyncReconciliation([], [live('a')], new Set(['a']), undefined)

expect(shouldSkipEmptyListing(0, 5, undefined, { sourceConfirmedEmpty: false })).toBe(true)
expect(result).toEqual({ resurrectIds: ['a'], softDeleteIds: [], hardDeleteIds: [] })
})
})

describe('exceedsDeletionSafetyThreshold', () => {
it('does not block a small deletion, even above 50%', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
it('leaves a document untouched when it is still present in the listing', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

expect(exceedsDeletionSafetyThreshold(4, 5, undefined, {})).toBe(false)
})

it('does not block a large deletion below 50%', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
const result = partitionSyncReconciliation([live('a')], [], new Set(['a']), undefined)

expect(exceedsDeletionSafetyThreshold(6, 20, undefined, {})).toBe(false)
expect(result).toEqual({ resurrectIds: [], softDeleteIds: [], hardDeleteIds: [] })
})

it('blocks a deletion above both the ratio and count thresholds by default', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
it('resurrects even on a forced fullSync', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

const result = partitionSyncReconciliation([], [live('a')], new Set(['a']), true)

expect(exceedsDeletionSafetyThreshold(10, 10, undefined, {})).toBe(true)
expect(exceedsDeletionSafetyThreshold(10, 10, undefined, undefined)).toBe(true)
expect(result.resurrectIds).toEqual(['a'])
})

it('does not block on a forced fullSync', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
it('hard-deletes both live and pending-removal documents immediately on a forced fullSync', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

expect(exceedsDeletionSafetyThreshold(10, 10, true, {})).toBe(false)
const result = partitionSyncReconciliation([live('a')], [live('b')], new Set(), true)

expect(result.softDeleteIds).toEqual([])
expect(result.hardDeleteIds.sort()).toEqual(['a', 'b'])
})

it('does not block when the connector confirms the deletion against the source', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
it('handles a mixed batch of every outcome in one pass', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

expect(exceedsDeletionSafetyThreshold(10, 10, undefined, { sourceConfirmedEmpty: true })).toBe(
false
const result = partitionSyncReconciliation(
[live('kept'), live('newly-missing')],
[live('resurrected'), live('confirmed-gone')],
new Set(['kept', 'resurrected']),
undefined
)

expect(result).toEqual({
resurrectIds: ['resurrected'],
softDeleteIds: ['newly-missing'],
hardDeleteIds: ['confirmed-gone'],
})
})

it('still blocks when sourceConfirmedEmpty is falsy', async () => {
const { exceedsDeletionSafetyThreshold } = await import(
'@/lib/knowledge/connectors/sync-engine'
)
it('ignores documents with a null externalId', async () => {
const { partitionSyncReconciliation } = await import('@/lib/knowledge/connectors/sync-engine')

expect(exceedsDeletionSafetyThreshold(10, 10, undefined, { sourceConfirmedEmpty: false })).toBe(
true
const result = partitionSyncReconciliation(
[live('a', null)],
[live('b', null)],
new Set(),
undefined
)

expect(result).toEqual({ resurrectIds: [], softDeleteIds: [], hardDeleteIds: [] })
})
})

Expand Down
Loading
Loading