Skip to content

Commit ae315cc

Browse files
icecrasher321claude
andcommitted
fix(forks): make the stale-plan probe best-effort
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>
1 parent 5d7749b commit ae315cc

2 files changed

Lines changed: 79 additions & 28 deletions

File tree

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,30 @@ describe('copyForkResourceContent', () => {
371371
})
372372
})
373373

374+
it('keeps a copied KB alive when the stale-plan probe fails', async () => {
375+
// The probe runs on every KB with referenced documents, but the state it repairs only exists
376+
// inside a rollout window. Letting it reach the KB catch would delete a complete copy and
377+
// clear every reference to it over a transient SELECT.
378+
dbChainMockFns.where.mockImplementationOnce(() => ({
379+
then: (resolve: (rows: unknown[]) => unknown) => resolve([{ total: 0 }]),
380+
}))
381+
dbChainMockFns.where.mockImplementationOnce(() => {
382+
throw new Error('stale-plan probe failed')
383+
})
384+
dbChainMockFns.limit.mockResolvedValueOnce([])
385+
386+
const result = await copyForkResourceContent({
387+
contentPlan: basePlan({
388+
knowledgeBases: [
389+
{ sourceId: 'src-kb', childId: 'child-kb', documentIdMap: { 'doc-1': 'child-doc-1' } },
390+
],
391+
}),
392+
requestId: 'test',
393+
})
394+
395+
expect(result).toEqual({ copied: 1, failed: 0, failures: [] })
396+
})
397+
374398
it('keeps a copied KB alive when the skipped-document count fails', async () => {
375399
// The count only feeds a log line. Letting it throw into the KB's catch would roll back a
376400
// 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: 55 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,6 +1054,58 @@ export async function copyForkResourceContent(params: {
10541054
})
10551055
}
10561056
}
1057+
/**
1058+
* Find the placeholders a worker from before this exclusion (a rolling deploy) planned for
1059+
* connector-managed documents, drop their persisted identities, and return the child ids to
1060+
* report as failed documents - the page query no longer returns their sources, so nothing
1061+
* would ever fill them, leaving archived empty rows that a mapping and a remapped
1062+
* `document-selector` still resolve to.
1063+
*
1064+
* Keyed on the SOURCE being connector-managed, which is deterministic: such a document can
1065+
* never become copyable, so this cannot race a concurrent attempt sitting between
1066+
* {@link ensureKbDocumentPlaceholder} and {@link finalizeKbDocument} (a "planned but unfilled"
1067+
* sweep would).
1068+
*
1069+
* Best-effort, like the count above: this probe runs on EVERY copied KB that has referenced
1070+
* documents, while the state it repairs exists only inside a rollout window. Letting a
1071+
* transient failure reach the KB's catch would delete an otherwise-complete copy and clear
1072+
* every reference to it - far worse, and far more likely, than the dangling placeholder it
1073+
* guards against. A failure is logged loudly and leaves that pre-existing state in place.
1074+
*/
1075+
const reconcileStalePlannedDocuments = async (kb: ForkContentKbEntry): Promise<string[]> => {
1076+
const plannedSourceIds = Object.keys(kb.documentIdMap)
1077+
if (plannedSourceIds.length === 0) return []
1078+
try {
1079+
const stalePlanned = await db
1080+
.select({ id: document.id })
1081+
.from(document)
1082+
.where(and(inArray(document.id, plannedSourceIds), isNotNull(document.connectorId)))
1083+
const staleChildIds: string[] = []
1084+
for (const { id } of stalePlanned) {
1085+
const childDocumentId = kb.documentIdMap[id]
1086+
if (!childDocumentId) continue
1087+
// Left in `documentIdMap` deliberately: if the KB itself later fails, its failure lists
1088+
// the same child id again, and the cleanup keys failed ids by kind in a Set.
1089+
await dropCopiedDocumentMapping(childDocumentId)
1090+
staleChildIds.push(childDocumentId)
1091+
logger.warn(
1092+
`[${requestId}] Dropping a fork placeholder planned for a connector-managed document`,
1093+
{ sourceDocumentId: id, childDocumentId, childKnowledgeBaseId: kb.childId }
1094+
)
1095+
}
1096+
return staleChildIds
1097+
} catch (error) {
1098+
logger.error(
1099+
`[${requestId}] Failed to reconcile fork placeholders planned for connector-managed documents`,
1100+
{
1101+
sourceKnowledgeBaseId: kb.sourceId,
1102+
childKnowledgeBaseId: kb.childId,
1103+
error: getErrorMessage(error),
1104+
}
1105+
)
1106+
return []
1107+
}
1108+
}
10571109
/**
10581110
* Report the connector-managed documents a copied KB leaves behind, since a fully
10591111
* connector-synced base lands in the child with no documents at all. Strictly observability,
@@ -1196,34 +1248,9 @@ export async function copyForkResourceContent(params: {
11961248
for (const kb of contentPlan.knowledgeBases) {
11971249
try {
11981250
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-
}
1251+
for (const childDocumentId of await reconcileStalePlannedDocuments(kb)) {
1252+
failedResources += 1
1253+
failures.push({ kind: 'knowledge-document', childId: childDocumentId })
12271254
}
12281255
let afterDocId: string | null = null
12291256
for (;;) {

0 commit comments

Comments
 (0)