fix(knowledge): replace deletion-safety heuristics with a two-phase tombstone#5884
fix(knowledge): replace deletion-safety heuristics with a two-phase tombstone#5884waleedlatif1 wants to merge 1 commit into
Conversation
…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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview On a normal sync, documents missing from the listing are soft-deleted ( The sync engine loads live and tombstoned rows, merges them for upsert classification, and applies resurrect / soft-delete / hard-delete after listing. Google Sheets no longer sets Reviewed by Cursor Bugbot for commit f0a00b8. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f0a00b8. Configure here.
| logger.info(`Resurrected ${resurrectIds.length} documents that reappeared at the source`, { | ||
| connectorId, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Resurrection runs after blocked updates
Medium Severity
When a pending-removal document reappears in the listing, sync classifies it as an update and runs updateDocument while deletedAt is still set. That helper only updates rows with null deletedAt, so content refresh fails and the sync counts a failure. deletedAt is cleared afterward in resurrection, leaving the document active with stale indexed content until a later sync.
Reviewed by Cursor Bugbot for commit f0a00b8. Configure here.
| logger.info( | ||
| `Marked ${softDeleteIds.length} documents pending removal — absent from source, confirming on next sync`, | ||
| { connectorId } | ||
| ) |
There was a problem hiding this comment.
Tombstoned docs escape connector cleanup
Medium Severity
This change marks absent connector documents pending removal by setting deletedAt. Connector teardown and sync-abort cleanup still select only rows with null deletedAt, so pending-removal documents are never hard-deleted. They can keep embeddings and storage after the connector is removed, with connectorId nulled by the foreign key.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f0a00b8. Configure here.
Greptile SummaryThis PR replaces immediate deletion heuristics with two-phase document tombstones.
Confidence Score: 3/5The PR should not merge until changed tombstoned documents can be updated successfully before they are made active again. A reappearing tombstone with changed content follows the existing-document update path, but persistence rejects deleted rows while later reconciliation still clears deletedAt, making stale content searchable. apps/sim/lib/knowledge/connectors/sync-engine.ts and apps/sim/lib/knowledge/connectors/sync-engine.test.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
L[Connector listing] --> P{Stored document present?}
P -->|Present and active| K[Keep or update]
P -->|Present and tombstoned| R[Resurrect]
P -->|Absent and active| F{Forced full sync?}
F -->|No| S[Set deletedAt tombstone]
F -->|Yes| H[Hard delete]
P -->|Absent and tombstoned| H
Reviews (1): Last reviewed commit: "fix(knowledge): replace deletion-safety ..." | Re-trigger Greptile |
| const priorByExternalId = new Map( | ||
| [...existingDocs, ...tombstonedDocs] | ||
| .filter((d) => d.externalId !== null) | ||
| .map((d) => [d.externalId!, d]) | ||
| ) |
There was a problem hiding this comment.
When a tombstoned document reappears with changed content, this map classifies it as an update even though persistence only accepts rows whose deletedAt is null. The failed update is handled per-document, but reconciliation still clears deletedAt because the external ID was seen, causing the old content and embeddings to become searchable again.
Knowledge Base Used: Knowledge (RAG) Module
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.
shouldSkipEmptyListing,exceedsDeletionSafetyThreshold, andsourceConfirmedEmptyentirely. 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.document.deletedAtcolumn as the tombstone marker — no schema migration.shouldReconcileDeletions(theisIncremental/listingCapped/listingTruncatedgate) is unchanged. Resurrection runs unconditionally even when that gate is closed, since presence is trustworthy evidence regardless of listing completeness.Type of Change
Testing
partitionSyncReconciliation(resurrect / soft-delete / hard-delete / mixed-batch / fullSync / null-externalId cases), replacing the removed heuristic tests.bun run lint,tsc --noEmit, and the fullconnectors+lib/knowledge+background/knowledge-connector-synctest suites (508 tests) all pass./cleanup's full 8-pass audit — no dead code, no stale/redundant comments, nothing to fix.Checklist