Skip to content

Commit a4a583f

Browse files
icecrasher321claude
andcommitted
fix(forks): clean up full-KB placeholders planned before the exclusion
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>
1 parent 678b5e9 commit a4a583f

2 files changed

Lines changed: 89 additions & 15 deletions

File tree

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

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

338+
it('drops a full-KB placeholder a pre-change worker planned for a connector-managed doc', async () => {
339+
// Rolling deploy: the fork tx ran on the old code and planned a placeholder for a
340+
// connector-managed document, which this worker's page query no longer returns. Nothing
341+
// would ever fill it, so it must be reported for cleanup rather than left archived behind a
342+
// live mapping that a remapped document-selector still resolves to.
343+
dbChainMockFns.where.mockImplementationOnce(() => ({
344+
// The skipped-document count.
345+
then: (resolve: (rows: unknown[]) => unknown) => resolve([{ total: 1 }]),
346+
}))
347+
dbChainMockFns.where.mockImplementationOnce(() => ({
348+
// The stale-plan probe: the planned source is connector-managed.
349+
then: (resolve: (rows: unknown[]) => unknown) => resolve([{ id: 'doc-1' }]),
350+
}))
351+
dbChainMockFns.limit.mockResolvedValueOnce([])
352+
353+
const result = await copyForkResourceContent({
354+
contentPlan: basePlan({
355+
knowledgeBases: [
356+
{ sourceId: 'src-kb', childId: 'child-kb', documentIdMap: { 'doc-1': 'child-doc-1' } },
357+
],
358+
documentMappingContext: { edgeChildWorkspaceId: 'edge-child-ws', sourceIsParent: false },
359+
}),
360+
requestId: 'test',
361+
})
362+
363+
expect(result.failed).toBe(1)
364+
expect(result.failures).toEqual([{ kind: 'knowledge-document', childId: 'child-doc-1' }])
365+
// The persisted identity goes too, or a later sync resolves to the row cleanup deletes.
366+
expect(mockDeleteCopiedResourceMappingsByTargets).toHaveBeenCalledWith({
367+
executor: expect.anything(),
368+
edgeChildWorkspaceId: 'edge-child-ws',
369+
sourceIsParent: false,
370+
targets: [{ resourceType: 'knowledge_document', resourceId: 'child-doc-1' }],
371+
})
372+
})
373+
338374
it('keeps a copied KB alive when the skipped-document count fails', async () => {
339375
// The count only feeds a log line. Letting it throw into the KB's catch would roll back a
340376
// perfectly good copy and clear every reference to it over a failed COUNT(*).

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

Lines changed: 53 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,29 @@ export async function copyForkResourceContent(params: {
10311031
billingContext ??= await resolveStorageBillingContext(childWorkspaceId)
10321032
return billingContext
10331033
}
1034+
/**
1035+
* Drop the persisted `knowledge_document` identity for a copied document that will not exist,
1036+
* so a later sync resolves the reference afresh instead of to a row the cleanup removes.
1037+
* Isolated like the rest of the post-commit phase: a mapping-cleanup failure is logged, never
1038+
* rethrown, since the caller is already reporting the document as failed.
1039+
*/
1040+
const dropCopiedDocumentMapping = async (childDocumentId: string): Promise<void> => {
1041+
const mappingContext = contentPlan.documentMappingContext
1042+
if (!mappingContext) return
1043+
try {
1044+
await deleteCopiedResourceMappingsByTargets({
1045+
executor: db,
1046+
edgeChildWorkspaceId: mappingContext.edgeChildWorkspaceId,
1047+
sourceIsParent: mappingContext.sourceIsParent,
1048+
targets: [{ resourceType: 'knowledge_document', resourceId: childDocumentId }],
1049+
})
1050+
} catch (mappingCleanupError) {
1051+
logger.error(`[${requestId}] Failed to clean mapping for a failed copied document`, {
1052+
childDocumentId,
1053+
error: getErrorMessage(mappingCleanupError),
1054+
})
1055+
}
1056+
}
10341057
/**
10351058
* Report the connector-managed documents a copied KB leaves behind, since a fully
10361059
* connector-synced base lands in the child with no documents at all. Strictly observability,
@@ -1173,6 +1196,35 @@ export async function copyForkResourceContent(params: {
11731196
for (const kb of contentPlan.knowledgeBases) {
11741197
try {
11751198
await logSkippedConnectorDocuments(kb)
1199+
// A worker from before this exclusion (a rolling deploy) could have planned a placeholder
1200+
// for a connector-managed document. The page query below no longer returns its source, so
1201+
// nothing would ever fill it - leaving an archived empty row that a persisted mapping and
1202+
// a remapped `document-selector` still resolve to. Report those child ids as failed
1203+
// documents instead, so the shared cleanup clears the references and drops the rows.
1204+
//
1205+
// Keyed on the SOURCE being connector-managed, which is deterministic: such a document can
1206+
// never become copyable, so this can never race a concurrent attempt mid-fill (unlike a
1207+
// "source is gone" check, which could).
1208+
const plannedSourceIds = Object.keys(kb.documentIdMap)
1209+
if (plannedSourceIds.length > 0) {
1210+
const stalePlanned = await db
1211+
.select({ id: document.id })
1212+
.from(document)
1213+
.where(and(inArray(document.id, plannedSourceIds), isNotNull(document.connectorId)))
1214+
for (const { id } of stalePlanned) {
1215+
const childDocumentId = kb.documentIdMap[id]
1216+
if (!childDocumentId) continue
1217+
// Left in `documentIdMap` deliberately: if the KB itself later fails, its failure
1218+
// lists the same child id again, and the cleanup keys failed ids by kind in a Set.
1219+
await dropCopiedDocumentMapping(childDocumentId)
1220+
failedResources += 1
1221+
failures.push({ kind: 'knowledge-document', childId: childDocumentId })
1222+
logger.warn(
1223+
`[${requestId}] Dropping a fork placeholder planned for a connector-managed document`,
1224+
{ sourceDocumentId: id, childDocumentId, childKnowledgeBaseId: kb.childId }
1225+
)
1226+
}
1227+
}
11761228
let afterDocId: string | null = null
11771229
for (;;) {
11781230
// Only copy LIVE documents - exclude soft-deleted and archived rows, matching
@@ -1353,21 +1405,7 @@ export async function copyForkResourceContent(params: {
13531405
})
13541406
copiedResources += 1
13551407
} catch (error) {
1356-
if (contentPlan.documentMappingContext) {
1357-
try {
1358-
await deleteCopiedResourceMappingsByTargets({
1359-
executor: db,
1360-
edgeChildWorkspaceId: contentPlan.documentMappingContext.edgeChildWorkspaceId,
1361-
sourceIsParent: contentPlan.documentMappingContext.sourceIsParent,
1362-
targets: [{ resourceType: 'knowledge_document', resourceId: docEntry.childDocId }],
1363-
})
1364-
} catch (mappingCleanupError) {
1365-
logger.error(`[${requestId}] Failed to clean mapping for a failed copied document`, {
1366-
childDocumentId: docEntry.childDocId,
1367-
error: getErrorMessage(mappingCleanupError),
1368-
})
1369-
}
1370-
}
1408+
await dropCopiedDocumentMapping(docEntry.childDocId)
13711409
failedResources += 1
13721410
failures.push({ kind: 'knowledge-document', childId: docEntry.childDocId })
13731411
logger.warn(`[${requestId}] Failed to copy document into mapped KB during sync`, {

0 commit comments

Comments
 (0)