Skip to content

Commit 6972149

Browse files
committed
fix(knowledge): check a tag slot under the lock that clears it
The guard added here read the definitions and then let the write happen in a later transaction that locks only the document row. Tag deletion locks the knowledge-base row and, inside it, clears the slot and drops the definition — so the two shared no lock and a deletion committing in that gap left a value in a slot nothing defines. The check now runs inside the writing transaction, after the same knowledge-base row lock every other writer here takes first. The guard already accepted a transaction and was written to be called this way. This also covers a case the earlier placement missed: an update given tag definition ids resolves them to slots before writing, and a deletion after that resolution stranded a value the pre-write check never looked at. It removes a lock-order inversion rather than adding one. Clearing a slot took document then embedding while an update took embedding then document, which is a deadlock window between two transactions that shared no lock. Taking the knowledge-base lock first orders them. The lock is taken only when an update actually writes a slot, so a rename, an enable, or the processing-status writes on the ingest path are unaffected. Note this narrows rather than introduces: before the guard, a single ordinary request could write an undefined slot with no concurrency at all.
1 parent fe7995c commit 6972149

5 files changed

Lines changed: 294 additions & 29 deletions

File tree

apps/sim/lib/knowledge/application/documents.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,8 @@ describe('knowledge document application use cases', () => {
596596
expect(mocks.updateDocument).toHaveBeenCalledWith(
597597
'document-1',
598598
{ filename: undefined, enabled: false },
599-
expect.any(String)
599+
expect.any(String),
600+
{ knowledgeBaseId: 'knowledge-1' }
600601
)
601602
expect(mocks.recordAudit).toHaveBeenCalledWith(
602603
expect.objectContaining({
@@ -675,7 +676,8 @@ describe('knowledge document application use cases', () => {
675676
number1: '2',
676677
boolean1: 'false',
677678
},
678-
expect.any(String)
679+
expect.any(String),
680+
{ knowledgeBaseId: 'knowledge-1' }
679681
)
680682
expect(mocks.recordAudit).toHaveBeenCalledWith(
681683
expect.objectContaining({

apps/sim/lib/knowledge/application/documents.ts

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ import {
6565
resolveKnowledgeTagFilters,
6666
toKnowledgeTagFilterConditions,
6767
} from '@/lib/knowledge/tags/filter-resolution'
68-
import { assertTagSlotsAreDefined, getDocumentTagDefinitions } from '@/lib/knowledge/tags/service'
68+
import { getDocumentTagDefinitions } from '@/lib/knowledge/tags/service'
6969
import { validateTagValue } from '@/lib/knowledge/tags/utils'
7070
import { StorageService } from '@/lib/uploads'
7171
import { generateKnowledgeBaseFileKey } from '@/lib/uploads/contexts/knowledge-base/knowledge-base-file-manager'
@@ -901,13 +901,6 @@ export const updateKnowledgeDocument = defineAuthorizedKnowledgeUseCase({
901901
updates,
902902
await resolveKnowledgeDocumentTagValueUpdates(context.knowledgeBaseId, input.tagValues)
903903
)
904-
} else {
905-
/**
906-
* `tagValues` addresses definitions by id and cannot name an undefined
907-
* slot; `input.updates` carries the raw `tag1`..`tag7` the API surfaces
908-
* accept, and can.
909-
*/
910-
await assertTagSlotsAreDefined(context.knowledgeBaseId, updates)
911904
}
912905
const updatedFields = Object.keys(updates).filter(
913906
(key) => updates[key as keyof typeof updates] !== undefined
@@ -917,7 +910,19 @@ export const updateKnowledgeDocument = defineAuthorizedKnowledgeUseCase({
917910
}
918911
return {
919912
kind: 'updated' as const,
920-
document: await updateDocument(context.documentId, updates, generateRequestId()),
913+
/**
914+
* `knowledgeBaseId` is what asks the write to refuse a value bound for a
915+
* slot this knowledge base has no definition for. Both branches above need
916+
* it: `input.updates` carries the raw `tag1`..`tag7` the API surfaces
917+
* accept and can name an undefined slot outright, and `tagValues`
918+
* addresses definitions by id but resolves them to slots before the write,
919+
* so a definition deleted after that resolution leaves the same stranded
920+
* value. The check runs inside the write's transaction under the knowledge
921+
* base's row lock, which is the lock tag deletion takes.
922+
*/
923+
document: await updateDocument(context.documentId, updates, generateRequestId(), {
924+
knowledgeBaseId: context.knowledgeBaseId,
925+
}),
921926
tagDefinitions: await getDocumentTagDefinitions(context.knowledgeBaseId),
922927
updatedFields,
923928
}

apps/sim/lib/knowledge/documents/service.ts

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ import {
122122
rebindKnowledgeDocumentSecretProvenance,
123123
replaceKnowledgeDocumentSecretProvenanceInTx,
124124
} from '@/lib/knowledge/secret-provenance'
125-
import { assertTagSlotsAreDefined } from '@/lib/knowledge/tags/service'
125+
import { assertTagSlotsAreDefined, writesTagSlots } from '@/lib/knowledge/tags/service'
126126
import {
127127
buildUndefinedTagsError,
128128
parseBooleanValue,
@@ -2675,14 +2675,15 @@ export async function createSingleDocument(
26752675
throw error
26762676
}
26772677
}
2678-
} else {
2679-
/**
2680-
* Only the slot-keyed branch needs this: `resolveDocumentTags` above already
2681-
* refuses a name it cannot resolve to a definition, so the name-keyed branch
2682-
* cannot reach an undefined slot.
2683-
*/
2684-
await assertTagSlotsAreDefined(knowledgeBaseId, processedTags)
26852678
}
2679+
/**
2680+
* Only the slot-keyed branch needs checking: `resolveDocumentTags` above
2681+
* already refuses a name it cannot resolve to a definition, so the name-keyed
2682+
* branch cannot reach an undefined slot. The check itself runs inside the
2683+
* transaction below, under the knowledge base's row lock, because tag
2684+
* deletion clears the slot and drops its definition under that same lock.
2685+
*/
2686+
const validateTagSlots = !documentData.documentTagsData
26862687

26872688
const newDocument = {
26882689
id: documentId,
@@ -2707,6 +2708,10 @@ export async function createSingleDocument(
27072708

27082709
await tx.execute(sql`SELECT 1 FROM knowledge_base WHERE id = ${knowledgeBaseId} FOR UPDATE`)
27092710

2711+
if (validateTagSlots) {
2712+
await assertTagSlotsAreDefined(knowledgeBaseId, processedTags, tx)
2713+
}
2714+
27102715
const kb = await tx
27112716
.select({
27122717
id: knowledgeBase.id,
@@ -3298,7 +3303,8 @@ export async function updateDocument(
32983303
boolean2?: string
32993304
boolean3?: string
33003305
},
3301-
requestId: string
3306+
requestId: string,
3307+
options?: { knowledgeBaseId?: string }
33023308
): Promise<{
33033309
id: string
33043310
knowledgeBaseId: string
@@ -3427,6 +3433,23 @@ export async function updateDocument(
34273433
})
34283434

34293435
const doc = await db.transaction(async (tx) => {
3436+
/**
3437+
* Taken before anything else this transaction touches, and only when the
3438+
* update lands a nonempty tag slot. Tag deletion clears the slot and drops
3439+
* its definition under this same knowledge-base row lock, so without it the
3440+
* check below is check-then-act and a deletion can commit between the check
3441+
* and the write, stranding a value in a slot nothing can name. The lock is
3442+
* skipped for tag-free updates (processing status, filename, enabled) so
3443+
* they never serialize on the knowledge base, and every other writer in this
3444+
* area takes this lock first too, so the order is unchanged.
3445+
*/
3446+
if (options?.knowledgeBaseId && writesTagSlots(updateData)) {
3447+
await tx.execute(
3448+
sql`SELECT 1 FROM knowledge_base WHERE id = ${options.knowledgeBaseId} FOR UPDATE`
3449+
)
3450+
await assertTagSlotsAreDefined(options.knowledgeBaseId, updateData, tx)
3451+
}
3452+
34303453
const hasTagUpdates = ALL_TAG_SLOTS.some((field) => typedUpdateData[field] !== undefined)
34313454

34323455
if (hasTagUpdates) {
Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Structural guard for the tag-slot check in `updateDocument`.
5+
*
6+
* The property under test is not an interleaving — a real race cannot be
7+
* reproduced against a mocked driver — but the shape that makes the
8+
* interleaving impossible: the check must run inside the write's transaction,
9+
* after the knowledge-base row lock tag deletion also takes, and before the
10+
* document row is written. Read outside that lock it is check-then-act, and a
11+
* tag deletion committing in between strands a value in a slot no definition
12+
* covers.
13+
*
14+
* The shared drizzle mock hands the transaction callback the same client as
15+
* `db`, so "ran against the transaction" cannot be asserted by identity; call
16+
* ordering relative to `db.transaction` is what pins it.
17+
*/
18+
import { document, knowledgeBase, knowledgeBaseTagDefinitions } from '@sim/db/schema'
19+
import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
20+
import { beforeEach, describe, expect, it, vi } from 'vitest'
21+
22+
const {
23+
mockApplyStorageUsageDeltasInTx,
24+
mockCheckStorageQuota,
25+
mockCheckStorageQuotaForBillingContext,
26+
mockDecrementStorageUsageForBillingContextInTx,
27+
mockIncrementStorageUsageForBillingContextInTx,
28+
mockMaybeNotifyStorageLimitForBillingContext,
29+
mockResolveStorageBillingContext,
30+
mockGetFileMetadataByKeys,
31+
mockEnqueueKnowledgeDocumentProcessing,
32+
} = vi.hoisted(() => ({
33+
mockApplyStorageUsageDeltasInTx: vi.fn(),
34+
mockCheckStorageQuota: vi.fn(),
35+
mockCheckStorageQuotaForBillingContext: vi.fn(),
36+
mockDecrementStorageUsageForBillingContextInTx: vi.fn(),
37+
mockIncrementStorageUsageForBillingContextInTx: vi.fn(),
38+
mockMaybeNotifyStorageLimitForBillingContext: vi.fn(),
39+
mockResolveStorageBillingContext: vi.fn(),
40+
mockGetFileMetadataByKeys: vi.fn(),
41+
mockEnqueueKnowledgeDocumentProcessing: vi.fn(),
42+
}))
43+
44+
vi.mock('@/lib/billing/storage', () => ({
45+
applyStorageUsageDeltasInTx: mockApplyStorageUsageDeltasInTx,
46+
checkStorageQuota: mockCheckStorageQuota,
47+
checkStorageQuotaForBillingContext: mockCheckStorageQuotaForBillingContext,
48+
decrementStorageUsageForBillingContextInTx: mockDecrementStorageUsageForBillingContextInTx,
49+
incrementStorageUsageForBillingContextInTx: mockIncrementStorageUsageForBillingContextInTx,
50+
maybeNotifyStorageLimitForBillingContext: mockMaybeNotifyStorageLimitForBillingContext,
51+
resolveStorageBillingContext: mockResolveStorageBillingContext,
52+
}))
53+
54+
vi.mock('@/lib/uploads/server/metadata', () => ({
55+
deleteFileMetadata: vi.fn(),
56+
getFileMetadataByKeys: mockGetFileMetadataByKeys,
57+
}))
58+
59+
vi.mock('@/lib/knowledge/documents/processing-outbox-event', () => ({
60+
enqueueKnowledgeDocumentProcessing: mockEnqueueKnowledgeDocumentProcessing,
61+
}))
62+
63+
import { createSingleDocument, updateDocument } from '@/lib/knowledge/documents/service'
64+
65+
const KNOWLEDGE_BASE_ID = 'kb-1'
66+
const NOW = new Date('2026-01-01T00:00:00.000Z')
67+
68+
/** invocationCallOrder of the first call to `spy` whose first argument is `table`. */
69+
function orderForTable(
70+
spy: { mock: { calls: unknown[][]; invocationCallOrder: number[] } },
71+
table: unknown
72+
): number {
73+
for (let i = 0; i < spy.mock.calls.length; i++) {
74+
if (spy.mock.calls[i][0] === table) return spy.mock.invocationCallOrder[i]
75+
}
76+
return -1
77+
}
78+
79+
function definition(tagSlot: string, displayName: string) {
80+
return {
81+
id: `tag-def-${tagSlot}`,
82+
knowledgeBaseId: KNOWLEDGE_BASE_ID,
83+
tagSlot,
84+
displayName,
85+
fieldType: 'text',
86+
createdAt: NOW,
87+
updatedAt: NOW,
88+
}
89+
}
90+
91+
describe('updateDocument tag-slot validation', () => {
92+
beforeEach(() => {
93+
vi.clearAllMocks()
94+
resetDbChainMock()
95+
queueTableRows(document, [
96+
{ id: 'doc-1', knowledgeBaseId: KNOWLEDGE_BASE_ID, secretProvenanceVersion: null },
97+
])
98+
dbChainMockFns.returning.mockResolvedValue([
99+
{ id: 'doc-1', knowledgeBaseId: KNOWLEDGE_BASE_ID, secretProvenanceVersion: null },
100+
])
101+
})
102+
103+
it('checks the slot inside the write transaction, under the knowledge-base row lock, before writing', async () => {
104+
queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
105+
106+
await updateDocument('doc-1', { tag1: 'priority' }, 'req-1', {
107+
knowledgeBaseId: KNOWLEDGE_BASE_ID,
108+
})
109+
110+
const transactionOrder = dbChainMockFns.transaction.mock.invocationCallOrder[0]
111+
const lockOrder = dbChainMockFns.execute.mock.invocationCallOrder[0] ?? -1
112+
const definitionReadOrder = orderForTable(dbChainMockFns.from, knowledgeBaseTagDefinitions)
113+
const documentWriteOrder = orderForTable(dbChainMockFns.update, document)
114+
115+
expect(transactionOrder).toBeGreaterThan(0)
116+
expect(lockOrder).toBeGreaterThan(transactionOrder)
117+
expect(definitionReadOrder).toBeGreaterThan(lockOrder)
118+
expect(documentWriteOrder).toBeGreaterThan(definitionReadOrder)
119+
})
120+
121+
it('refuses a slot no definition covers, and writes neither the document nor its embeddings', async () => {
122+
queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
123+
124+
await expect(
125+
updateDocument('doc-1', { tag2: 'purple' }, 'req-1', {
126+
knowledgeBaseId: KNOWLEDGE_BASE_ID,
127+
})
128+
).rejects.toMatchObject({ code: 'validation' })
129+
130+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
131+
})
132+
133+
it('takes no knowledge-base lock for an update that lands no tag value', async () => {
134+
await updateDocument('doc-1', { filename: 'renamed.txt' }, 'req-1', {
135+
knowledgeBaseId: KNOWLEDGE_BASE_ID,
136+
})
137+
138+
expect(dbChainMockFns.execute).not.toHaveBeenCalled()
139+
expect(orderForTable(dbChainMockFns.from, knowledgeBase)).toBe(-1)
140+
expect(orderForTable(dbChainMockFns.update, document)).toBeGreaterThan(0)
141+
})
142+
143+
it('takes no knowledge-base lock when clearing a slot, so a stranded value stays erasable', async () => {
144+
await updateDocument('doc-1', { tag2: '' }, 'req-1', { knowledgeBaseId: KNOWLEDGE_BASE_ID })
145+
146+
expect(dbChainMockFns.execute).not.toHaveBeenCalled()
147+
expect(orderForTable(dbChainMockFns.update, document)).toBeGreaterThan(0)
148+
})
149+
})
150+
151+
describe('createSingleDocument tag-slot validation', () => {
152+
beforeEach(() => {
153+
vi.clearAllMocks()
154+
resetDbChainMock()
155+
dbChainMockFns.limit.mockResolvedValue([
156+
{ id: KNOWLEDGE_BASE_ID, workspaceId: 'workspace-1', userId: 'knowledge-owner' },
157+
])
158+
mockResolveStorageBillingContext.mockResolvedValue({
159+
workspaceId: 'workspace-1',
160+
billedAccountUserId: 'workspace-owner',
161+
billingEntity: { type: 'organization' as const, id: 'workspace-org' },
162+
plan: 'team_25000',
163+
customStorageLimitGB: null,
164+
})
165+
mockCheckStorageQuotaForBillingContext.mockResolvedValue({ allowed: true })
166+
mockIncrementStorageUsageForBillingContextInTx.mockResolvedValue(5)
167+
mockApplyStorageUsageDeltasInTx.mockResolvedValue(undefined)
168+
mockMaybeNotifyStorageLimitForBillingContext.mockResolvedValue(undefined)
169+
mockGetFileMetadataByKeys.mockResolvedValue([])
170+
mockEnqueueKnowledgeDocumentProcessing.mockResolvedValue('outbox-1')
171+
})
172+
173+
const documentData = {
174+
filename: 'note.txt',
175+
fileUrl: 'data:text/plain;base64,SGVsbG8=',
176+
fileSize: 5,
177+
mimeType: 'text/plain',
178+
}
179+
180+
it('checks the slot inside the insert transaction, under the knowledge-base row lock', async () => {
181+
queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
182+
183+
await createSingleDocument({ ...documentData, tag1: 'priority' }, KNOWLEDGE_BASE_ID, 'req-1')
184+
185+
const transactionOrder = dbChainMockFns.transaction.mock.invocationCallOrder[0]
186+
const lockOrder = dbChainMockFns.execute.mock.invocationCallOrder[0] ?? -1
187+
const definitionReadOrder = orderForTable(dbChainMockFns.from, knowledgeBaseTagDefinitions)
188+
const documentInsertOrder = orderForTable(dbChainMockFns.insert, document)
189+
190+
expect(transactionOrder).toBeGreaterThan(0)
191+
expect(lockOrder).toBeGreaterThan(transactionOrder)
192+
expect(definitionReadOrder).toBeGreaterThan(lockOrder)
193+
expect(documentInsertOrder).toBeGreaterThan(definitionReadOrder)
194+
})
195+
196+
it('refuses a slot no definition covers, and inserts nothing', async () => {
197+
queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
198+
199+
await expect(
200+
createSingleDocument({ ...documentData, tag2: 'purple' }, KNOWLEDGE_BASE_ID, 'req-1')
201+
).rejects.toMatchObject({ code: 'validation' })
202+
203+
expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
204+
})
205+
})

0 commit comments

Comments
 (0)