fix(forks): stop copying connector-managed knowledge base documents - #6818
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview This PR excludes connector-managed documents ( Rolling deploy: stale placeholders planned before the change are reconciled (mapping dropped, failures reported); probe/count helpers are best-effort so transient DB errors do not roll back a good KB copy. Docs add a warn callout that fully connector-synced KBs fork with no documents, plus workarounds (re-add connector or download/upload). Documents whose connector was deleted in the source still copy ( Reviewed by Cursor Bugbot for commit ae315cc. Configure here. |
|
@cursor review |
Greptile SummaryThe PR prevents connector-managed knowledge documents from being detached and duplicated across workspace forks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts | Consistently excludes connector-managed documents across planning and copying while handling mixed-version placeholders and non-critical diagnostic failures. |
| apps/sim/ee/workspace-forking/lib/copy/copy-resources.test.ts | Adds focused tests for connector filtering, stale-plan reconciliation, and best-effort diagnostic behavior. |
| apps/docs/content/docs/en/platform/enterprise/forks.mdx | Clearly documents that connector-synced documents do not cross fork boundaries and explains available alternatives. |
Reviews (4): Last reviewed commit: "fix(forks): make the stale-plan probe be..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@greptile the last two rounds both reviewed |
|
@cursor review |
A fork copies a KB's documents but never its connectors, so a connector-sourced document arrives with `connector_id` nulled and its `external_id` intact. The sync engine keys every existing/tombstone/ exclusion lookup off `connector_id`, so that copy is invisible to it - never updated, reconciled, or purged - and `doc_connector_external_id_idx` does not constrain it either, since its `connector_id` is NULL. Attaching a connector in the child then re-ingests every page as a NEW row on top of the snapshot. Each fork hop re-copies the previous hop's orphans and adds one more generation, so a prod -> UAT -> staging chain leaves three rows per page and a knowledge search returns the same page three times, one of them serving content frozen at the fork date. Exclude connector-managed documents from all four doors a document can enter a fork through: the whole-KB content copy, the in-transaction placeholder pre-creation, the sync-only copy into an already-mapped KB, and the content fill (guarded for payloads planned by a pre-change worker mid-rollout). The placeholder path matters as much as the copy loop - filtering only the content phase would leave a permanently archived row behind a persisted `knowledge_document` mapping. Skipped on both sides, the reference clears like any other uncopied document's. A document whose connector was deleted already has a null `connector_id` (the FK is ON DELETE SET NULL) and is static in the source too, so it still copies. One count(*) per copied KB logs what was left behind, since a fully connector-synced KB now forks to zero documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The connector-managed count feeds a log line, but it sat inside the KB's try block, so a transient failure on a COUNT(*) would roll back a copy that had otherwise succeeded and clear every reference to it. Move it into a helper that swallows its own error. Counting is not copying: only the copy itself may fail a resource. Test proven red by removing the catch - the mutation reports a knowledge-base failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mapped-KB fill guarded a pre-change plan, but the full-KB path did not: a placeholder planned by an old worker for a connector-managed document is simply no longer returned by the page query, so nothing fills it and it stays archived behind a live mapping that a remapped document-selector still resolves to. Report those child ids as failed documents so the shared cleanup clears their references and drops the rows, and delete their persisted identity so a later sync does not resolve to a row cleanup removes. Keyed on the SOURCE being connector-managed, which can never become copyable, so it cannot race a concurrent attempt mid-fill the way a "source is gone" check could. The mapping drop is now one helper shared with the mapped-KB catch. Test proven red by removing the reconciliation block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe ran inside the KB try, so a transient SELECT would reach the catch, roll back a complete copy, delete the child base, and clear every reference to it. Weighing it as "load-bearing, so fail closed" was wrong: the probe runs on EVERY copied KB that has referenced documents, while the state it repairs exists only inside a rollout window. Failing closed traded a common-path outage against a rare-squared one. It now swallows its own failure with a loud error log, leaving that pre-existing state in place rather than destroying a good copy. Test proven red by removing the catch - the mutation reports the KB failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a4a583f to
ae315cc
Compare
|
@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 ae315cc. Configure here.
1 participant
Summary
A fork copies a knowledge base's documents but never its connectors — deliberate, per
copyForkResourceContainers("the child is a content snapshot without live sync"). What was not deliberate is that connector-sourced documents were copied anyway, arriving withconnector_idnulled andexternal_id,content_hash,source_urland the originaluploaded_atintact.That copy is unreachable by the sync engine, which scopes every existing / tombstone / exclusion lookup to
eq(document.connectorId, connectorId)(sync-engine.ts:828-885). It can never be updated, reconciled, or purged.doc_connector_external_id_idx—UNIQUE (connector_id, external_id) WHERE deleted_at IS NULL— does not constrain it either, because Postgres treats NULLs as distinct.So attaching a connector in the child re-ingests every page as a new row on top of the snapshot. And since the copy loop filtered only
deleted_at/archived_at, each hop also re-copied the previous hop's orphans:fork_document_*)Three rows per page, so a knowledge search returns the same page three times — one of them frozen at the fork date, serving content nothing will ever refresh. This was reported from a real prod → UAT → staging chain: 191 documents for ~56 Confluence pages.
The change
Exclude connector-managed documents at all four doors a document can enter a fork through:
copyForkResourceContentwhole-KB page queryisNull(document.connectorId)createForkDocumentPlaceholders(in the fork tx)isNull(document.connectorId)planForkMappedKbDocumentCopies(sync-only, doc → already-mapped KB)isNull(document.connectorId)contentPlan.documentsfill loopsource.connectorIdguardThe placeholder path matters as much as the copy loop. Filtering only the content phase would leave a referenced connector document with an archived placeholder and a persisted
knowledge_documentmapping pointing at it, with nothing behind it to ever fill it. Skipped on both sides, itsdocument-selectorreference clears like any other uncopied document's — existing, documented behavior.The fourth guard covers one case only: a payload planned by a pre-change worker and filled by a new one mid-rollout. It throws into the existing per-document cleanup, which drops the placeholder and clears its references — the same outcome the new planner produces.
A document whose connector was deleted in the source already has a null
connector_id(the FK isON DELETE SET NULL) and is static content there too, so it still copies. That falls out of the predicate rather than needing a special case.One
count(*)per copied KB logs what was left behind — a fully connector-synced KB now forks to zero documents, and without this there would be nothing anywhere explaining why.Docs
New
#### Connector-synced documents are not copiedunder "Knowledge bases and documents": the sharp edge in awarncallout, the reasoning, and the two workarounds (re-add the connector for live content; download-and-upload for a frozen snapshot). The at-a-glance row links to it.Also corrected the adjacent line, which claimed "Documents that the forked workflows actually reference are included" — the fork copies every document in the KB; only the placeholder pre-creation was ever reference-scoped.
Not in scope
This stops new duplication; it does not clean up KBs already in this state. No follow-up is needed — the known-affected knowledge bases have already been corrected manually, out of band. No backfill or migration ships with this PR.
Type of Change
Testing
bun run test ee/workspace-forking→ 39 files, 601 tests passed.type-check,biome checkandcheck:api-validationall clean.Four new tests. The predicates are the whole fix, and the row-queue mocks return whatever is enqueued regardless of the
where, so three of them assert the condition tree viaflattenMockConditions+schemaMock— the helper that exists for exactly this:connector_idknowledge_documentmappingknowledge-documentfailure for cleanupAlso flipped the
planForkMappedKbDocumentCopiesfixture fromconnectorId: 'connector-1'tonull— it was describing a document the planner must now never place.Reviewers should focus on the placeholder/content-phase agreement (they must skip the same set or a mapping dangles) and on whether losing connector content at fork time is the right trade for the affected teams — a fully-synced KB now forks empty, which is a real behavior change the docs callout is there to set expectations for.
Checklist