fix(knowledge): replace deletion-safety heuristics with a two-phase tombstone by waleedlatif1 · Pull Request #5884 · simstudioai/sim

waleedlatif1 · 2026-07-23T06:55:30Z

Summary

Follow-up to #5883, which merged before this redesign was ready. #5883 fixed the immediate bug (trashed Google Sheets tabs never purging on a normal sync) with sourceConfirmedEmpty — a connector-set flag bypassing two static safety heuristics (shouldSkipEmptyListing, exceedsDeletionSafetyThreshold). On reflection that approach was the wrong shape: a boolean escape hatch that any future connector could set to bypass both deletion safety nets at once, with no protection against the source itself being transiently wrong.

This PR replaces it with a proper two-phase tombstone, matching how production sync systems actually solve this (Microsoft Entra Connect's deletion threshold, SCIM/Entra ID's deferred-delete deprovisioning, Cassandra/Couchbase tombstones): 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), not purged immediately.
It's only hard-deleted once a later sync confirms it's still absent.
If it reappears in between, it's resurrected automatically — self-heals a transient outage or bad API response without needing to distinguish "real" emptiness from "ambiguous" emptiness at all.
A forced full sync still purges everything absent in one pass — the existing "trigger a full sync to force cleanup" escape hatch is preserved.
Removes shouldSkipEmptyListing, exceedsDeletionSafetyThreshold, and sourceConfirmedEmpty entirely. Google Sheets no longer needs any connector-specific bypass — it goes through the exact same general path as all 8 connectors, with zero special-casing and zero misuse surface for future connectors.
Uses the existing (previously unused for individual documents) document.deletedAt column as the tombstone marker — no schema migration.
shouldReconcileDeletions (the isIncremental/listingCapped/listingTruncated gate) is unchanged. Resurrection runs unconditionally even when that gate is closed, since presence is trustworthy evidence regardless of listing completeness.

Type of Change

Bug fix / design improvement

Testing

Added 7 new unit tests for partitionSyncReconciliation (resurrect / soft-delete / hard-delete / mixed-batch / fullSync / null-externalId cases), replacing the removed heuristic tests.
bun run lint, tsc --noEmit, and the full connectors + lib/knowledge + background/knowledge-connector-sync test suites (508 tests) all pass.
Ran /cleanup's full 8-pass audit — no dead code, no stale/redundant comments, nothing to fix.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

vercel · 2026-07-23T06:55:36Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Jul 23, 2026 6:50pm

cursor · 2026-07-23T06:55:40Z

PR Summary

High Risk
Changes core knowledge connector sync deletion, resurrection, and hard-delete semantics with new race handling against connector detach; incorrect tombstone or ownership logic could hide, wrongly delete, or corrupt standalone KB documents.

Overview
Replaces connector deletion safety heuristics (shouldSkipEmptyListing, exceedsDeletionSafetyThreshold, and connector sourceConfirmedEmpty) with a two-phase tombstone flow using document.deletedAt.

Sync reconciliation: Documents missing from a listing are soft-deleted (pending removal) on the first observation and hard-deleted only if a later sync still does not see them; reappearance resurrects them. Forced full sync still purges absent docs in one pass. New helpers partitionSyncReconciliation, shouldRunIncrementalSync (forces full listing while recent tombstones exist), and filterStillOwnedReconciliationIds drive this; resurrection is not blocked when incremental reconciliation is gated off.

Safety during races: Reconciliation and stuck-doc retry take a knowledge_connector FOR UPDATE lock and re-check connectorId. hardDeleteDocuments accepts optional expectedConnectorId with a second verify inside the delete transaction. updateDocument clears deletedAt on content updates and requires matching connectorId.

Connector delete API: When keeping documents, tombstoned rows are cleared (deletedAt null); document selection includes pending-removal docs.

Google Sheets: Stops setting sourceConfirmedEmpty on trashed empty listings; trashed spreadsheets rely on the same tombstone path as other connectors.

^{Reviewed by Cursor Bugbot for commit a5c804f. Configure here.}

greptile-apps · 2026-07-23T06:59:13Z

Greptile Summary

Replaces heuristic deletion safeguards with two-phase tombstone reconciliation.

Soft-deletes documents after their first confirmed absence and hard-deletes them after a later confirmation.
Resurrects reappearing documents while preventing failed or unusable refreshes from exposing stale content.
Forces full listings while recent tombstones require confirmation and preserves one-pass deletion for forced full syncs.
Serializes reconciliation and retry operations against connector deletion and revalidates document ownership.
Removes the Google Sheets connector-specific empty-source bypass.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failures remain.

Important Files Changed

Filename	Overview
apps/sim/lib/knowledge/connectors/sync-engine.ts	Implements tombstone partitioning, verified resurrection, full-listing selection, ownership-safe reconciliation, and serialized stuck-document retries.
apps/sim/lib/knowledge/documents/service.ts	Adds optional connector-ownership revalidation to batched hard deletion.
apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/route.ts	Includes tombstoned documents when deleting a connector and restores retained documents before connector detachment.
apps/sim/connectors/google-sheets/google-sheets.ts	Removes the connector-specific confirmed-empty bypass and delegates deletion handling to general tombstone reconciliation.
apps/sim/lib/knowledge/connectors/sync-engine.test.ts	Replaces heuristic tests with coverage for incremental selection, reconciliation outcomes, failed refreshes, and ownership filtering.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Connector listing] --> B{Stored document present?}
  B -->|Present and live| C[Keep or update]
  B -->|Present and tombstoned| D{Refresh verified?}
  D -->|Yes| E[Resurrect]
  D -->|No| F[Keep tombstoned for retry]
  B -->|Absent and live| G{Forced full sync?}
  G -->|No| H[Soft-delete]
  G -->|Yes| I[Hard-delete]
  B -->|Absent and tombstoned| I

_{Reviews (12): Last reviewed commit: "fix(knowledge): lock the connector row f..." | Re-trigger Greptile}

waleedlatif1 · 2026-07-23T15:44:46Z

@greptile

waleedlatif1 · 2026-07-23T15:44:48Z

@cursor review

waleedlatif1 · 2026-07-23T15:52:41Z

@greptile

waleedlatif1 · 2026-07-23T15:52:42Z

@cursor review

waleedlatif1 · 2026-07-23T16:01:11Z

@greptile

waleedlatif1 · 2026-07-23T16:01:12Z

@cursor review

waleedlatif1 · 2026-07-23T17:31:27Z

@greptile

waleedlatif1 · 2026-07-23T17:31:28Z

@cursor review

waleedlatif1 · 2026-07-23T17:38:09Z

@greptile

waleedlatif1 · 2026-07-23T17:38:09Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 4b14143. Configure here.}

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit f875d4e. Configure here.}

…ombstone 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.

…ned 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.

…efresh 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.

…rom 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.

…xist 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.

…s 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.

…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.

…g-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.

…ip-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.

…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.

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.

waleedlatif1 · 2026-07-23T18:27:44Z

@greptile

waleedlatif1 · 2026-07-23T18:27:46Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ddc52a4. Configure here.}

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.

waleedlatif1 · 2026-07-23T18:42:07Z

@greptile

waleedlatif1 · 2026-07-23T18:42:09Z

@cursor review

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.

waleedlatif1 · 2026-07-23T18:51:15Z

@greptile

waleedlatif1 · 2026-07-23T18:51:17Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit a5c804f. Configure here.}

waleedlatif1 mentioned this pull request Jul 23, 2026

fix(knowledge): purge trashed Google Sheets tabs on a normal sync #5883

Merged

6 tasks