Skip to content

Commit 678b5e9

Browse files
icecrasher321claude
andcommitted
fix(forks): keep the skipped-document count from failing a copied KB
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>
1 parent 9335ba5 commit 678b5e9

2 files changed

Lines changed: 61 additions & 36 deletions

File tree

apps/sim/ee/workspace-forking/lib/copy/copy-resources.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,24 @@ describe('copyForkResourceContent', () => {
335335
).toBe(true)
336336
})
337337

338+
it('keeps a copied KB alive when the skipped-document count fails', async () => {
339+
// The count only feeds a log line. Letting it throw into the KB's catch would roll back a
340+
// perfectly good copy and clear every reference to it over a failed COUNT(*).
341+
dbChainMockFns.where.mockImplementationOnce(() => {
342+
throw new Error('count failed')
343+
})
344+
dbChainMockFns.limit.mockResolvedValueOnce([])
345+
346+
const result = await copyForkResourceContent({
347+
contentPlan: basePlan({
348+
knowledgeBases: [{ sourceId: 'src-kb', childId: 'child-kb', documentIdMap: {} }],
349+
}),
350+
requestId: 'test',
351+
})
352+
353+
expect(result).toEqual({ copied: 1, failed: 0, failures: [] })
354+
})
355+
338356
it('uses the blob content digest so a retry cannot adopt an older failed snapshot', async () => {
339357
dbChainMockFns.limit
340358
.mockResolvedValueOnce([sourceDoc])

apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,39 @@ export async function copyForkResourceContent(params: {
10311031
billingContext ??= await resolveStorageBillingContext(childWorkspaceId)
10321032
return billingContext
10331033
}
1034+
/**
1035+
* Report the connector-managed documents a copied KB leaves behind, since a fully
1036+
* connector-synced base lands in the child with no documents at all. Strictly observability,
1037+
* so it swallows its own failure: counting is not copying, and a transient error here must not
1038+
* take down the KB the way a failed document does.
1039+
*/
1040+
const logSkippedConnectorDocuments = async (kb: ForkContentKbEntry): Promise<void> => {
1041+
try {
1042+
const [row] = await db
1043+
.select({ total: sql<number>`count(*)` })
1044+
.from(document)
1045+
.where(
1046+
and(
1047+
eq(document.knowledgeBaseId, kb.sourceId),
1048+
isNotNull(document.connectorId),
1049+
isNull(document.deletedAt),
1050+
isNull(document.archivedAt)
1051+
)
1052+
)
1053+
const skipped = Number(row?.total ?? 0)
1054+
if (skipped === 0) return
1055+
logger.info(`[${requestId}] Skipped connector-managed documents in a copied knowledge base`, {
1056+
sourceKnowledgeBaseId: kb.sourceId,
1057+
childKnowledgeBaseId: kb.childId,
1058+
skipped,
1059+
})
1060+
} catch (error) {
1061+
logger.warn(`[${requestId}] Failed to count the documents a copied knowledge base skipped`, {
1062+
sourceKnowledgeBaseId: kb.sourceId,
1063+
error: getErrorMessage(error),
1064+
})
1065+
}
1066+
}
10341067

10351068
for (const table of contentPlan.tables) {
10361069
try {
@@ -1139,48 +1172,22 @@ export async function copyForkResourceContent(params: {
11391172

11401173
for (const kb of contentPlan.knowledgeBases) {
11411174
try {
1142-
// Connector-managed documents are excluded from the copy (see the predicate below), and a
1143-
// KB can be entirely connector-sourced - so report what was left behind rather than letting
1144-
// the child silently land with fewer documents than the source.
1145-
const [{ skipped: connectorManaged = 0 } = {}] = await db
1146-
.select({ skipped: sql<number>`count(*)` })
1147-
.from(document)
1148-
.where(
1149-
and(
1150-
eq(document.knowledgeBaseId, kb.sourceId),
1151-
isNotNull(document.connectorId),
1152-
isNull(document.deletedAt),
1153-
isNull(document.archivedAt)
1154-
)
1155-
)
1156-
if (Number(connectorManaged) > 0) {
1157-
logger.info(
1158-
`[${requestId}] Skipped connector-managed documents in a copied knowledge base`,
1159-
{
1160-
sourceKnowledgeBaseId: kb.sourceId,
1161-
childKnowledgeBaseId: kb.childId,
1162-
skipped: Number(connectorManaged),
1163-
}
1164-
)
1165-
}
1175+
await logSkippedConnectorDocuments(kb)
11661176
let afterDocId: string | null = null
11671177
for (;;) {
11681178
// Only copy LIVE documents - exclude soft-deleted and archived rows, matching
11691179
// how the rest of the KB system treats them as gone (chunks/tags/search filter
11701180
// both). A fork must not resurrect documents removed from the source base.
11711181
//
1172-
// Connector-managed documents (`connector_id IS NOT NULL`) are excluded too. A copy can
1173-
// only be a DETACHED snapshot - the child gets no connector (see
1174-
// `copyForkResourceContainers`), and the sync engine keys every existing/tombstone/
1175-
// exclusion lookup off `connector_id`, so a detached copy is invisible to it: it can
1176-
// never be updated, reconciled, or purged, and `doc_connector_external_id_idx`
1177-
// (UNIQUE on `(connector_id, external_id)`) does not constrain it because its
1178-
// `connector_id` is NULL. Attaching a connector to the child then re-ingests every
1179-
// page as a NEW row on top of the snapshot, stacking one duplicate generation per fork
1180-
// hop and returning the same page several times from one retrieval. Skipping them
1181-
// instead leaves the child's connector as the single owner of that content. A source
1182-
// document whose connector was DELETED already has a null `connector_id` (the FK is
1183-
// ON DELETE SET NULL) and is static content in the source too, so it still copies.
1182+
// Connector-managed documents are excluded too, because a copy could only ever be a
1183+
// DETACHED snapshot: the child gets no connector (see `copyForkResourceContainers`), and
1184+
// the sync engine keys every existing/tombstone/exclusion lookup off `connector_id`, so
1185+
// the copy is invisible to it - never updated, reconciled, or purged. Attaching a
1186+
// connector in the child then re-ingests every page as a NEW row on top of the snapshot,
1187+
// stacking one dead generation per fork hop. Skipping them leaves the child's own
1188+
// connector as the single owner of that content. A source document whose connector was
1189+
// DELETED already has a null `connector_id` (the FK is ON DELETE SET NULL) and is static
1190+
// content in the source too, so it still copies.
11841191
const liveDocs = and(
11851192
eq(document.knowledgeBaseId, kb.sourceId),
11861193
isNull(document.connectorId),

0 commit comments

Comments
 (0)