fix(knowledge): purge trashed Google Sheets tabs on a normal sync#5883
Merged
Conversation
Trashing a spreadsheet made listDocuments return an empty listing, but the sync engine's zero-document guard skips deletion reconciliation whenever a listing comes back empty and documents already exist — it can't tell a genuinely empty source apart from a provider outage. For a single-spreadsheet connector, trashing its one source item empties the entire listing, so the guard always fired and the stale tabs never got cleaned up on a normal sync, contradicting the documented behavior. Add shouldSkipEmptyListing, mirroring shouldReconcileDeletions: a connector can now set syncContext.sourceConfirmedEmpty when it has positively confirmed the empty result against the source (not merely inferred it from an empty listing page), letting reconciliation proceed. The Google Sheets connector sets this flag when it confirms the spreadsheet is trashed via a direct Drive metadata lookup. No other connector sets it, so this doesn't change behavior anywhere else.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Connectors can set Reviewed by Cursor Bugbot for commit 147572d. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR purges indexed Google Sheets tabs during a normal sync after the source spreadsheet is trashed. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(knowledge): let sourceConfirmedEmpty..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…n safety threshold The zero-document guard bypass alone wasn't enough: for a trashed spreadsheet with more than 5 tabs, reconciliation would proceed but the separate mass-deletion ratio guard (>50% deleted, >5 docs) still blocked the actual delete on a normal sync, requiring a forced full resync anyway. Extracted the ratio guard into exceedsDeletionSafetyThreshold, mirroring shouldSkipEmptyListing, so a connector's positive source confirmation bypasses both guards consistently.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ 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 147572d. Configure here.
Collaborator
Author
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…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.
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…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.
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…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.
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…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.
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…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.
waleedlatif1
added a commit
that referenced
this pull request
Jul 23, 2026
…ombstone (#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shouldSkipEmptyListingtosync-engine.ts, mirroring the existingshouldReconcileDeletionshelper: a connector can setsyncContext.sourceConfirmedEmptyto vouch that it positively confirmed the empty result against the source (not merely inferred it from an empty listing page). Google Sheets sets this flag when its direct Drive metadata lookup confirms the spreadsheet is trashed.Type of Change
Testing
shouldSkipEmptyListingcovering: non-empty listing, no existing docs, forced fullSync, default skip behavior, and the newsourceConfirmedEmptyoverride (including a falsy-value case).sourceConfirmedEmptyis set only on the trashed path, and left unset on the absent/false/fail-open paths.bun run lint,tsc --noEmit, and the fullconnectors+lib/knowledge/connectorstest suites (411 tests) all pass.Checklist