Skip to content

Commit 7fb91e3

Browse files
committed
fix(knowledge): re-dispatch an import whose first dispatch was lost, and remove a base whose tag setup fails
- the base takes the import's id and a worker claims the import before working, so a second dispatch exits untouched - a retried completion dispatches again an import no worker has claimed - a worker that finds its import settled before any claim deletes the unused bundle - tag-definition setup that fails removes the still-empty base
1 parent 27600ef commit 7fb91e3

9 files changed

Lines changed: 219 additions & 56 deletions

File tree

‎apps/sim/lib/knowledge/__integration__/bundle-import.integration.ts‎

Lines changed: 86 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,15 @@ async function stageBundleBytes(importId: string, bytes: Buffer): Promise<string
172172
}
173173

174174
async function runImport(importId: string, sourceKey: string, name: string) {
175-
await runKnowledgeImport({
175+
await runKnowledgeImport(await importPayload(importId, sourceKey, name))
176+
const record = await findKnowledgeImport({ importId, assertedWorkspaceId: ids.workspaceId })
177+
if (!record) throw new Error('Import record disappeared')
178+
return record
179+
}
180+
181+
async function importPayload(importId: string, sourceKey: string, name: string) {
182+
return {
176183
importId,
177-
knowledgeBaseId: generateId(),
178184
workspaceId: ids.workspaceId,
179185
userId: ids.aliceId,
180186
name,
@@ -183,10 +189,7 @@ async function runImport(importId: string, sourceKey: string, name: string) {
183189
actorUserId: ids.aliceId,
184190
workspaceId: ids.workspaceId,
185191
}),
186-
})
187-
const record = await findKnowledgeImport({ importId, assertedWorkspaceId: ids.workspaceId })
188-
if (!record) throw new Error('Import record disappeared')
189-
return record
192+
}
190193
}
191194

192195
async function importedDocuments(knowledgeBaseId: string) {
@@ -482,6 +485,27 @@ describe('knowledge base bundle import', () => {
482485
).rejects.toThrow()
483486
})
484487

488+
it('lets exactly one of two concurrent workers run an import', async () => {
489+
const importId = generateId()
490+
const sourceKey = await stageExportedBundle(importId)
491+
const payload = await importPayload(importId, sourceKey, 'Claimed fixture')
492+
493+
await Promise.all([runKnowledgeImport(payload), runKnowledgeImport(payload)])
494+
495+
expect(await findKnowledgeImport({ importId })).toMatchObject({
496+
status: 'completed',
497+
knowledgeBaseId: importId,
498+
documentsImported: 2,
499+
documentsFailed: 0,
500+
})
501+
expect(
502+
await db
503+
.select({ id: knowledgeBase.id })
504+
.from(knowledgeBase)
505+
.where(eq(knowledgeBase.name, 'Claimed fixture'))
506+
).toEqual([{ id: importId }])
507+
})
508+
485509
it('fails without a partial base when the name is already taken', async () => {
486510
const importId = generateId()
487511
const sourceKey = await stageExportedBundle(importId)
@@ -583,6 +607,62 @@ describe('knowledge base import requests', () => {
583607
.toEqual([{ resourceId: finished.knowledgeBaseId }])
584608
})
585609

610+
it('starts an import whose first dispatch was lost when its completion is retried', async () => {
611+
const bytes = await exportedBundleBytes()
612+
const created = await createKnowledgeImport.execute({
613+
principal: session,
614+
input: {
615+
body: {
616+
workspaceId: ids.workspaceId,
617+
name: 'Redispatched import',
618+
source: {
619+
type: 'upload',
620+
name: 'fixture.simkb.zip',
621+
contentType: 'application/zip',
622+
size: bytes.length,
623+
},
624+
},
625+
},
626+
request,
627+
})
628+
await writeLocalPutObject({
629+
uploadId: created.upload.id,
630+
key: created.upload.finalKey,
631+
body: new Blob([new Uint8Array(bytes)]).stream(),
632+
expectedSize: bytes.length,
633+
contentType: created.upload.contentType,
634+
metadata: uploadSessionObjectMetadata(created.upload),
635+
})
636+
// The record a completion wrote before the request died, with no worker behind it.
637+
await insertKnowledgeImport({
638+
id: created.upload.id,
639+
workspaceId: ids.workspaceId,
640+
userId: ids.aliceId,
641+
sourceKey: created.upload.storageKey,
642+
})
643+
const input = {
644+
importId: created.import.id,
645+
workspaceId: ids.workspaceId,
646+
uploadToken: created.upload.uploadToken,
647+
}
648+
649+
await completeKnowledgeImport.execute({ principal: session, input, request })
650+
651+
await expect
652+
.poll(
653+
async () =>
654+
(await readKnowledgeImport.execute({ principal: session, input })).import.status,
655+
{ interval: 50, timeout: 10_000 }
656+
)
657+
.toBe('completed')
658+
expect(
659+
await db
660+
.select({ id: knowledgeBase.id })
661+
.from(knowledgeBase)
662+
.where(eq(knowledgeBase.name, 'Redispatched import'))
663+
).toEqual([{ id: created.import.id }])
664+
})
665+
586666
it('refuses a bundle that is not a zip archive before creating an upload', async () => {
587667
const importUploads = () =>
588668
db

‎apps/sim/lib/knowledge/application/imports.ts‎

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,24 @@ function findStartedImport(context: KnowledgeImportUploadContext) {
223223
})
224224
}
225225

226+
/** What the runner needs, with usage admitted for the caller completing the upload. */
227+
async function importPayload(
228+
principal: Principal,
229+
context: KnowledgeImportUploadContext
230+
): Promise<KnowledgeImportPayload> {
231+
const { billingAttribution, userId } = await assertKnowledgeUsageAdmitted(principal, context)
232+
const { upload } = context
233+
const name = importedName(upload)
234+
return {
235+
importId: upload.id,
236+
workspaceId: context.workspaceId,
237+
userId,
238+
...(name ? { name } : {}),
239+
sourceKey: upload.storageKey,
240+
billingAttribution,
241+
}
242+
}
243+
226244
/**
227245
* Writes the durable record while the upload is being completed, before it
228246
* counts as complete. From then on the stale-import janitor owns the bundle, so
@@ -235,34 +253,27 @@ async function recordKnowledgeImport(
235253
principal: Principal,
236254
context: KnowledgeImportUploadContext
237255
): Promise<KnowledgeImportPayload | null> {
238-
const { billingAttribution, userId } = await assertKnowledgeUsageAdmitted(principal, context)
239-
const { upload } = context
256+
const payload = await importPayload(principal, context)
240257
const inserted = await insertKnowledgeImport({
241-
id: upload.id,
242-
workspaceId: context.workspaceId,
243-
userId,
244-
sourceKey: upload.storageKey,
258+
id: payload.importId,
259+
workspaceId: payload.workspaceId,
260+
userId: payload.userId,
261+
sourceKey: payload.sourceKey,
262+
})
263+
return inserted ? payload : null
264+
}
265+
266+
function startKnowledgeImport(payload: KnowledgeImportPayload): Promise<void> {
267+
return dispatchBackgroundTask<typeof knowledgeImportTask>('knowledge-import', payload, {
268+
runInProcess: runKnowledgeImport,
269+
trigger: { tags: [`workspaceId:${payload.workspaceId}`, `importId:${payload.importId}`] },
245270
})
246-
if (!inserted) return null
247-
const name = importedName(upload)
248-
return {
249-
importId: upload.id,
250-
knowledgeBaseId: generateId(),
251-
workspaceId: context.workspaceId,
252-
userId,
253-
...(name ? { name } : {}),
254-
sourceKey: upload.storageKey,
255-
billingAttribution,
256-
}
257271
}
258272

259273
/** Starts the runner; a dispatch that fails settles the import and drops its bundle. */
260274
async function dispatchKnowledgeImport(payload: KnowledgeImportPayload): Promise<void> {
261275
try {
262-
await dispatchBackgroundTask<typeof knowledgeImportTask>('knowledge-import', payload, {
263-
runInProcess: runKnowledgeImport,
264-
trigger: { tags: [`workspaceId:${payload.workspaceId}`, `importId:${payload.importId}`] },
265-
})
276+
await startKnowledgeImport(payload)
266277
} catch (error) {
267278
await finishKnowledgeImport(payload.importId, {
268279
status: 'failed',
@@ -280,6 +291,29 @@ async function dispatchKnowledgeImport(payload: KnowledgeImportPayload): Promise
280291
}
281292
}
282293

294+
/**
295+
* Dispatches again an import whose record no worker has claimed: its first
296+
* dispatch was lost with the request that wrote the record, or is still
297+
* queued. Either way a second run is safe, since only one worker can claim it,
298+
* and a failure here leaves the import to the stale-import janitor rather than
299+
* failing one that may already be on its way.
300+
*/
301+
async function redispatchUnclaimedImport(
302+
principal: Principal,
303+
context: KnowledgeImportUploadContext,
304+
record: KnowledgeImportRecord
305+
): Promise<void> {
306+
if (record.status !== 'running' || record.startedAt) return
307+
try {
308+
await startKnowledgeImport(await importPayload(principal, context))
309+
} catch (error) {
310+
logger.warn('Could not dispatch an unclaimed knowledge import again', {
311+
importId: record.id,
312+
error: getErrorMessage(error, 'Unknown error'),
313+
})
314+
}
315+
}
316+
283317
export const createKnowledgeImport = defineAuthorizedKnowledgeUseCase({
284318
operation: knowledgeOperations.importCreate,
285319
resolveContext: ({ input }: { input: CreateKnowledgeImportInput }) =>
@@ -367,7 +401,10 @@ export const completeKnowledgeImport = defineAuthorizedKnowledgeUseCase({
367401
}) => resolveUploadContext(principal, input),
368402
async execute({ principal, context }): Promise<CompleteKnowledgeImportResult> {
369403
const existing = await findStartedImport(context)
370-
if (existing) return { import: fromRecord(existing), startedKnowledgeBaseId: null }
404+
if (existing) {
405+
await redispatchUnclaimedImport(principal, context, existing)
406+
return { import: fromRecord(existing), startedKnowledgeBaseId: null }
407+
}
371408
let started: KnowledgeImportPayload | null
372409
try {
373410
const completed = await completeUploadSession({
@@ -405,7 +442,7 @@ export const completeKnowledgeImport = defineAuthorizedKnowledgeUseCase({
405442
importId: context.upload.id,
406443
assertedWorkspaceId: context.workspaceId,
407444
})
408-
return { import: fromRecord(record), startedKnowledgeBaseId: started?.knowledgeBaseId ?? null }
445+
return { import: fromRecord(record), startedKnowledgeBaseId: started?.importId ?? null }
409446
},
410447
/** Recorded once per import: a repeated completion only reads back the one already started. */
411448
projectAudit: ({ context, result }) =>

‎apps/sim/lib/knowledge/transfer/import-jobs.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import { db } from '@sim/db'
99
import { knowledgeBaseImport } from '@sim/db/schema'
1010
import { truncate } from '@sim/utils/string'
11-
import { and, eq } from 'drizzle-orm'
11+
import { and, eq, isNull } from 'drizzle-orm'
1212
import { OrchestrationError } from '@/lib/core/orchestration/types'
1313

1414
/** Where an import's uploaded bundle is stored until the import settles. */
@@ -73,6 +73,20 @@ export async function getKnowledgeImport(params: {
7373
return record
7474
}
7575

76+
/**
77+
* Claims a running import for one worker. Returns `false` when another worker
78+
* already claimed it, or it settled, so a duplicate dispatch exits untouched.
79+
*/
80+
export async function claimKnowledgeImport(importId: string): Promise<boolean> {
81+
const now = new Date()
82+
const claimed = await db
83+
.update(knowledgeBaseImport)
84+
.set({ startedAt: now, updatedAt: now })
85+
.where(and(whileRunning(importId), isNull(knowledgeBaseImport.startedAt)))
86+
.returning({ id: knowledgeBaseImport.id })
87+
return claimed.length > 0
88+
}
89+
7690
/**
7791
* Records the runner's progress, which doubles as its heartbeat. Returns
7892
* `false` once the import is no longer running, which tells the runner to stop.

‎apps/sim/lib/knowledge/transfer/import-runner.ts‎

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ import {
3030
openKnowledgeBundle,
3131
} from '@/lib/knowledge/transfer/import-archive'
3232
import {
33+
claimKnowledgeImport,
34+
findKnowledgeImport,
3335
finishKnowledgeImport,
3436
KNOWLEDGE_IMPORT_STORAGE_CONTEXT,
3537
type KnowledgeImportFailure,
@@ -50,10 +52,13 @@ const logger = createLogger('KnowledgeImportRunner')
5052
*/
5153
const DOCUMENT_CONCURRENCY = 2
5254

55+
/**
56+
* What a worker needs to run an import. The base it creates takes the import's
57+
* id, so every dispatch of one import names the same base, and the import's
58+
* audit names it before it exists.
59+
*/
5360
export interface KnowledgeImportPayload {
5461
importId: string
55-
/** Minted when the upload completed, so the audit of the import names this base. */
56-
knowledgeBaseId: string
5762
workspaceId: string
5863
/** The person the base and its documents are attributed to. */
5964
userId: string
@@ -239,7 +244,7 @@ async function importBundle(
239244
if (!(await updateRunningKnowledgeImport(payload.importId, {}))) return
240245
const knowledgeBase = await createAuthorizedKnowledgeBase(
241246
{
242-
id: payload.knowledgeBaseId,
247+
id: payload.importId,
243248
name: payload.name ?? manifest.knowledgeBase.name,
244249
description: manifest.knowledgeBase.description ?? undefined,
245250
workspaceId: payload.workspaceId,
@@ -262,16 +267,22 @@ async function importBundle(
262267
return
263268
}
264269

265-
for (const tag of manifest.tags) {
266-
await createTagDefinition(
267-
{
268-
knowledgeBaseId: knowledgeBase.id,
269-
tagSlot: tag.slot,
270-
displayName: tag.displayName,
271-
fieldType: tag.fieldType,
272-
},
273-
requestId
274-
)
270+
try {
271+
for (const tag of manifest.tags) {
272+
await createTagDefinition(
273+
{
274+
knowledgeBaseId: knowledgeBase.id,
275+
tagSlot: tag.slot,
276+
displayName: tag.displayName,
277+
fieldType: tag.fieldType,
278+
},
279+
requestId
280+
)
281+
}
282+
} catch (error) {
283+
// No document has landed yet, so a base without its tags is removed rather than kept half-built.
284+
await deleteKnowledgeBase(knowledgeBase.id, requestId)
285+
throw error
275286
}
276287

277288
const context: ImportDocumentContext = {
@@ -332,11 +343,18 @@ async function importBundle(
332343
}
333344

334345
/**
335-
* Runs one import to a settled state. The bundle is copied to local disk so
336-
* its entries can be read by offset, and both copies are removed however the
337-
* import ends.
346+
* Runs one import to a settled state. The worker claims the import first, so a
347+
* second dispatch of it leaves the first worker's bundle alone. The bundle is
348+
* copied to local disk so its entries can be read by offset, and both copies
349+
* are removed however the import ends.
338350
*/
339351
export async function runKnowledgeImport(payload: KnowledgeImportPayload): Promise<void> {
352+
if (!(await claimKnowledgeImport(payload.importId))) {
353+
// Settled before any worker claimed it, so no worker is reading the bundle.
354+
const record = await findKnowledgeImport({ importId: payload.importId })
355+
if (record && !record.startedAt) await deleteBundle(payload)
356+
return
357+
}
340358
const directory = await mkdtemp(join(tmpdir(), 'knowledge-import-'))
341359
let reader: KnowledgeBundleReader | undefined
342360
try {
@@ -365,13 +383,17 @@ export async function runKnowledgeImport(payload: KnowledgeImportPayload): Promi
365383
} finally {
366384
reader?.close()
367385
await rm(directory, { recursive: true, force: true })
368-
await deleteFile({ key: payload.sourceKey, context: KNOWLEDGE_IMPORT_STORAGE_CONTEXT }).catch(
369-
(error: unknown) => {
370-
logger.warn('Failed to delete an imported knowledge bundle', {
371-
importId: payload.importId,
372-
error: getErrorMessage(error, 'Unknown error'),
373-
})
374-
}
375-
)
386+
await deleteBundle(payload)
376387
}
377388
}
389+
390+
async function deleteBundle(payload: KnowledgeImportPayload): Promise<void> {
391+
await deleteFile({ key: payload.sourceKey, context: KNOWLEDGE_IMPORT_STORAGE_CONTEXT }).catch(
392+
(error: unknown) => {
393+
logger.warn('Failed to delete an imported knowledge bundle', {
394+
importId: payload.importId,
395+
error: getErrorMessage(error, 'Unknown error'),
396+
})
397+
}
398+
)
399+
}

‎packages/db/migrations/0436_knowledge_base_import.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ CREATE TABLE "knowledge_base_import" (
1313
"failures" jsonb DEFAULT '[]'::jsonb NOT NULL,
1414
"error" text,
1515
"created_at" timestamp DEFAULT now() NOT NULL,
16+
"started_at" timestamp,
1617
"updated_at" timestamp DEFAULT now() NOT NULL,
1718
"completed_at" timestamp
1819
);

0 commit comments

Comments
 (0)