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

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

PR #5883 merged an interim fix (sourceConfirmedEmpty bypassing two
static safety heuristics) before this follow-up redesign was ready.
This supersedes that approach with a properly general fix, matching
how production sync systems (Entra Connect, SCIM/Entra ID
deprovisioning, Cassandra/Couchbase tombstones) handle this exact
problem: never let a single observation trigger an irreversible mass
deletion, no matter how confident the signal looks.

A document missing from a normal sync's listing is now soft-deleted
(marked pending-removal) rather than hard-deleted immediately. It's
only actually purged once a *later* sync confirms it's still absent.
If it reappears in between, it's resurrected automatically — this
self-heals a transient outage or a bad API response without needing
to distinguish 'real' emptiness from 'ambiguous' emptiness at all,
which is what the removed heuristics were trying (and failing) to do
from a single observation.

This removes shouldSkipEmptyListing, exceedsDeletionSafetyThreshold,
and sourceConfirmedEmpty entirely — Google Sheets no longer needs a
connector-specific bypass flag, since a genuinely trashed spreadsheet
now reconciles through the exact same general path as every other
connector, with no special-casing and no new misuse surface for
future connectors. A forced fullSync still purges everything absent
in one pass, preserving the existing 'trigger a full sync to force
cleanup' escape hatch.

Uses the existing (previously unused for individual documents)
document.deletedAt column as the tombstone marker — no schema
migration required. shouldReconcileDeletions (the isIncremental /
listingCapped / listingTruncated gate) is unchanged; it still governs
whether reconciliation may run at all. Resurrection runs
unconditionally even when that gate is closed, since presence is
trustworthy evidence regardless of whether the listing was complete.

* fix(knowledge): resurrect atomically on content update, sweep tombstoned docs on connector teardown

Two real gaps found by review, both stemming from the same root
cause: other code paths assumed deletedAt IS NULL means 'the only
real rows' and were never updated for the new tombstone semantics.

- updateDocument's guard required deletedAt IS NULL, so a tombstoned
  document reappearing with CHANGED content failed its content
  update (rejected by the guard) while the separate resurrect step
  still cleared deletedAt regardless — the document became active
  again but kept serving stale pre-tombstone content. Fixed by
  clearing deletedAt as part of the same update statement and
  dropping the guard, so content and resurrection land atomically.
- Both connector-teardown cleanup paths (the ConnectorDeletedException
  handler in sync-engine.ts, and the connector DELETE API route) only
  swept documents with deletedAt IS NULL, so pending-removal documents
  escaped cleanup entirely and were orphaned once their connector was
  gone. Fixed by including tombstoned docs in both sweeps — there's no
  future sync left to confirm or resurrect them once the connector
  itself is deleted.

* fix(knowledge): don't resurrect a tombstoned document whose content refresh failed

Critical race found by adversarial audit: resurrectIds was derived
purely from 'externalId was seen in the listing', independent of
whether the paired content update actually succeeded. If updateDocument
threw (hydration failure, storage upload failure, or any other
transient error) or the deferred-hydration fetch itself failed, the
document's row was never touched — yet the separate unconditional
resurrect step still cleared deletedAt for it anyway, reproducing the
exact bug this PR fixes (visible again, serving stale pre-tombstone
content), just triggered by a failed write instead of a gated one.

Track externalIds whose refresh attempt failed (hydration rejection or
write rejection) and exclude them from partitionSyncReconciliation's
resurrectIds. A failed refresh leaves the document tombstoned as-is —
not soft-deleted, not hard-deleted, not resurrected — so a later sync
gets a clean retry instead of the row landing in an inconsistent state
either way.

* fix(knowledge): exclude fulfilled-but-unverified hydration outcomes from resurrection too

Both bots independently found a third instance of the same bug class:
when deferred hydration for an update fulfills but has no usable
content (skipped as oversized, or an empty re-fetch), the code falls
back to keeping the stored content as last-known-good and counts it
unchanged — correct for an already-visible document, but for a
tombstoned one it means content was never actually verified as
current. That fallback wasn't added to failedExternalIds, so
reconciliation still resurrected it with stale pre-tombstone content
despite hydration never actually confirming anything.

Fixed by adding both fallback branches to failedExternalIds, same as
the rejected-promise cases. Verified across every connector's
skippedReason call site that this only ever happens inside
getDocument (the deferred hydration path already covered here) —
no connector sets skippedReason directly in listDocuments, so there's
no equivalent listing-time gap to fix.

* fix(knowledge): force a full listing when pending-removal documents exist

A subtler instance of the same bug class, this time architectural
rather than a code-path gap: an incremental listing only includes
documents whose content changed since the last sync. A tombstoned
document that's still genuinely present at the source but unchanged
would never appear in an incremental delta at all, so it could never
be resurrected — and on a connector that runs incrementally from here
on (its normal syncMode), it would stay tombstoned indefinitely with
no self-correcting path, only a manual full resync.

Added shouldRunIncrementalSync (extracted as a testable pure function,
matching shouldReconcileDeletions' existing pattern) and a cheap
existence check for any pending-removal document on this connector.
Whenever one exists, this sync forces a full listing instead of an
incremental one, guaranteeing every tombstoned document gets a real
resurrect-or-confirm decision. This only affects which listing mode
runs — it doesn't touch options.fullSync, so the deletion-safety grace
period for other, unrelated documents in the same sync is unaffected.

* fix(knowledge): bound tombstone-forced full syncs, resurrect kept docs on connector delete

Two more real findings from this review round:

- A document whose refresh keeps failing every sync (e.g. permanently
  oversized) never resurrects and never hard-deletes (it's present in
  the listing, just unreadable) — correct on its own, but it also never
  stops being counted by hasTombstonedDocs, so it would force a full
  listing for this connector forever, permanently disabling incremental
  sync on account of one stuck document. Bounded the check to the same
  RETRY_WINDOW_DAYS already used for the stuck-document retry sweep
  below: past the window, this connector stops forcing full syncs on
  the stuck document's account. The document itself is unaffected —
  it stays tombstoned either way, matching the existing 'last-known-good
  forever' tolerance this codebase already accepts for any document
  whose hydration keeps failing.

- The connector-DELETE route's deleteDocuments=false path (kept docs)
  counted tombstoned documents but never resolved them one way or the
  other. With the connector gone, there's no future sync left to ever
  confirm or resurrect them, so they'd become permanent invisible
  orphans holding storage forever. Since 'kept' documents become normal
  standalone KB entries once detached from their connector, resurrect
  any pending-removal ones as part of that transition — consistent
  with what happens to their non-tombstoned sibling documents.

* fix(knowledge): serialize reconciliation writes against a concurrent connector delete

An independent adversarial audit (not just Greptile/Cursor) found the
one genuinely critical gap 6 rounds of bot review missed: resurrect/
soft-delete/hard-delete writes applied raw document IDs snapshotted at
the top of the sync, with no re-check immediately before the write. A
connector-DELETE request choosing to keep documents detaches them
(connectorId set to NULL) via the exact same FOR UPDATE lock on the
connector row that this fix now also takes before applying any
reconciliation write — serializing the two: whichever transaction
commits first wins, and the loser's re-check sees the up-to-date
connectorId and skips any document the other request already claimed.
Without this, a sync racing a 'delete connector, keep documents'
request could silently resurrect-then-strand or soft/hard-delete a
document the user explicitly chose to keep, with a secondary effect of
misclassifying it for storage-billing decrement (which keys off
whether connectorId is still set).

Also tightened the excludedDocs query: it previously required
deletedAt IS NULL, so a document that was both userExcluded and
tombstoned (reachable via excludeConnectorDocuments, which has no
deletedAt filter) fell out of the exclusion set and could be silently
un-excluded and re-indexed on reappearing. Dropped that requirement so
userExcluded is honored regardless of tombstone state, consistent with
how existingDocs/tombstonedDocs are already merged for classification.

Documented (not code-changed) the remaining lower-severity finding: a
document that outlives the 7-day hasTombstonedDocs bound on a
persistently-incremental connector can stay unresolved indefinitely.
Deliberately not hard-deleting it after the window expires — that
would delete a document with no positive evidence it's actually gone,
reintroducing the exact risk this whole design exists to avoid. It's
already fully excluded from search/billing/listings either way, so
this is an accepted, bounded, orphaned-row trade-off, not a
correctness or security issue.

* fix(knowledge): close remaining hard-delete race window, guard listing-time skip/drop resurrection

Two more real findings, both closing gaps in the previous round's fixes:

- The FOR UPDATE lock protected resurrect/soft-delete (applied inside
  the same transaction) but hardDeleteDocuments still ran after that
  transaction committed, using IDs snapshotted under the lock. A
  concurrent 'delete connector, keep documents' request could still
  detach those same documents in the gap between commit and the
  hardDeleteDocuments call. Added an optional expectedConnectorId
  parameter to hardDeleteDocuments/hardDeleteDocumentBatch — when
  provided, it re-verifies connectorId at the moment of the actual
  delete query, not just the caller's earlier snapshot. Every other
  caller is unaffected (parameter is optional, defaults to no filter).

- Two more listing-time paths could resurrect a tombstoned document
  without ever verifying its content: a listing-time skippedReason
  short-circuits classification straight to 'unchanged' before the
  hash comparison ever runs, and empty non-deferred content classifies
  as 'drop' unconditionally regardless of hash. Both are now added to
  failedExternalIds when reappearing on an existing (possibly
  tombstoned) document, same treatment as the deferred-hydration
  equivalents from the prior round.

* refactor(knowledge): dedupe retry-cutoff computation, extract ownership-filter helper

/simplify pass: hoist the shared RETRY_WINDOW_DAYS cutoff into one computation
reused by both the tombstone-retry bound and the stuck-document retry query,
and pull the FOR-UPDATE-lock reconciliation's ID filtering into a pure,
directly-unit-tested filterStillOwnedReconciliationIds function matching this
file's existing convention for its other decision logic.

* fix(knowledge): re-verify connectorId at the actual hard-delete, fix docsDeleted count

Cursor findings: expectedConnectorId was only checked on the pre-transaction
SELECT in hardDeleteDocumentBatch, not on the DELETE itself — the billing
lookups and KB locking in between are async and can span a concurrent
"delete connector, keep documents" request, so the delete (and its embedding
cleanup) now re-verifies against a fresh in-transaction snapshot instead of
the stale existingIds. Also fixed docsDeleted to use hardDeleteDocuments'
actual returned count instead of the pre-filter candidate count, so a sync
log no longer overreports deletions that expectedConnectorId skipped.

/cleanup: dropped two comments that only restated the line below them.

* fix(knowledge): re-verify connectorId in updateDocument's atomic write

Greptile P1: updateDocument's content-update/resurrect write only checked
document.id and archivedAt, never connectorId — despite connectorId being
a parameter — so a document a concurrent "delete connector, keep documents"
request already detached could still be matched, resurrected, and
overwritten with connector-sourced content after the connector was deleted.
Adds the same connectorId ownership check already used by the reconciliation
transaction and hardDeleteDocuments in this PR.

* fix(knowledge): close connectorId race in the stuck-document retry path

Final independent audit: the stuck-document retry block selected candidate
IDs filtered by connectorId, then reset their processing state and deleted
their embeddings using that stale ID set with no re-check — the same
SELECT-then-write race already patched in updateDocument and
hardDeleteDocumentBatch. A concurrent "delete connector, keep documents"
request could null out connectorId in between, so this now re-verifies
ownership immediately before the embedding delete/document update/re-enqueue
and only acts on documents still owned by this connector.

* fix(knowledge): lock the connector row for stuck-doc retry ownership too

Cursor + Greptile (same root cause, two reports): the previous round's fix
re-checked connectorId via a separate SELECT before the embedding delete and
processing-state reset, but a bare re-SELECT only narrows a TOCTOU window, it
never closes it — a concurrent "delete connector, keep documents" request
could still commit its detach in between. Wraps the ownership re-check and
both writes in a transaction that takes the same knowledge_connector FOR
UPDATE lock the DELETE route takes before nulling connectorId, so the two
requests serialize instead of racing, matching the pattern already used by
the reconciliation transaction elsewhere in this PR.

Author: waleedlatif1

apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/route.ts

@@ -328,23 +328,25 @@ export const DELETE = withRouteHandler(async (request: NextRequest, { params }:
     const { deletedDocs, docCount } = await db.transaction(async (tx) => {
       await tx.execute(sql`SELECT 1 FROM knowledge_connector WHERE id = ${connectorId} FOR UPDATE`)
 
+      // Includes pending-removal (tombstoned) docs — the connector is being
+      // deleted, so there's no future sync left to confirm or resurrect them.
       const docs = await tx
         .select({ id: document.id, fileUrl: document.fileUrl })
         .from(document)
-        .where(
-          and(
-            eq(document.connectorId, connectorId),
-            isNull(document.archivedAt),
-            isNull(document.deletedAt)
-          )
-        )
+        .where(and(eq(document.connectorId, connectorId), isNull(document.archivedAt)))
 
+      const documentIds = docs.map((doc) => doc.id)
       if (deleteDocuments) {
-        const documentIds = docs.map((doc) => doc.id)
         if (documentIds.length > 0) {
           await tx.delete(embedding).where(inArray(embedding.documentId, documentIds))
           await tx.delete(document).where(inArray(document.id, documentIds))
         }
+      } else if (documentIds.length > 0) {
+        // Kept documents become normal standalone KB entries once their connector
+        // is gone — resurrect any pending-removal ones rather than leaving them
+        // invisible tombstones with no future sync left to ever confirm or
+        // resurrect them.
+        await tx.update(document).set({ deletedAt: null }).where(inArray(document.id, documentIds))
       }
 
       const deletedConnectors = await tx

apps/sim/connectors/google-sheets/google-sheets.test.ts

@@ -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 () => {
@@ -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 () => {
@@ -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', () => {

apps/sim/connectors/google-sheets/google-sheets.ts

@@ -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) {
@@ -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 }
     }