From 367aff4834c96046218d77c9ac530d2a97ab23de Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 15 Aug 2026 11:47:49 -0700 Subject: [PATCH 1/2] fix(knowledge): bound chunking separators so one config can't stall processing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `chunkingStrategyOptionsSchema.separators` accepted an arbitrary-length array of arbitrary-length strings, next to a `pattern` field already capped at 500 chars. `RecursiveChunker` splits the whole document once per separator and walks the list from the top for every oversized fragment, so a persisted config with thousands of non-matching separators cost seconds of synchronous CPU on every later document upload — work neither the processing `Promise.race` timeout nor the after-the-fact chunk-count cap can interrupt. Measured on a 21.3 MB document: 632 ms at 100 separators, 6.1 s at 1000, 36.8 s at 5000. - Bound `separators` to 32 entries of at most 100 characters on the write path. The largest built-in recipe (markdown) uses 16, so hand-tuned lists still fit. - Keep the stored/read shape tolerant, so a config written before the bound still lists instead of failing response validation. - Clamp in `RecursiveChunker` too, with a warning, so an already-persisted oversized list cannot reach the split loop. An over-long separator is dropped rather than truncated: a truncated separator matches where the configured one never did, silently re-cutting the document, while dropping it behaves like a separator that finds no match. A list left empty falls back to the recipe. - Walk non-matching separators iteratively instead of recursing, so stack depth no longer tracks the separator count. Verified behavior-preserving against the previous implementation over 4000 randomized configs — byte-identical output. - Validate in the create-base modal so the limit surfaces inline. After the fix the same 21.3 MB document costs ~300 ms at every separator count. --- .../create-base-modal/create-base-modal.tsx | 33 ++++++-- .../lib/api/contracts/knowledge/base.test.ts | 77 +++++++++++++++++++ apps/sim/lib/api/contracts/knowledge/base.ts | 34 +++++++- apps/sim/lib/chunkers/constants.ts | 17 ++++ .../lib/chunkers/recursive-chunker.test.ts | 58 ++++++++++++++ apps/sim/lib/chunkers/recursive-chunker.ts | 63 +++++++++++---- 6 files changed, 260 insertions(+), 22 deletions(-) create mode 100644 apps/sim/lib/api/contracts/knowledge/base.test.ts create mode 100644 apps/sim/lib/chunkers/constants.ts diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx b/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx index d9f123c4ea7..bf0b1c349f7 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx @@ -25,6 +25,7 @@ import { getErrorMessage } from '@sim/utils/errors' import { useParams } from 'next/navigation' import { type FieldErrors, useForm } from 'react-hook-form' import { z } from 'zod' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from '@/lib/chunkers/constants' import type { StrategyOptions } from '@/lib/chunkers/types' import { KNOWLEDGE_BASE_DESCRIPTION_MAX_LENGTH } from '@/lib/knowledge/constants' import { @@ -57,6 +58,14 @@ const STRATEGY_OPTIONS = [ { value: 'regex', label: 'Regex (custom pattern)' }, ] as const +/** Splits the comma-separated separator field into the list the API receives. */ +function parseSeparators(value: string | undefined): string[] { + if (!value?.trim()) return [] + return value + .split(',') + .map((separator) => separator.trim().replace(/\\n/g, '\n').replace(/\\t/g, '\t')) +} + const STRATEGY_COMBOBOX_OPTIONS: ComboboxOption[] = STRATEGY_OPTIONS.map((o) => ({ label: o.label, value: o.value, @@ -124,6 +133,20 @@ const FormSchema = z path: ['regexPattern'], } ) + .refine((data) => parseSeparators(data.customSeparators).length <= MAX_CHUNKING_SEPARATORS, { + message: `At most ${MAX_CHUNKING_SEPARATORS} separators are allowed`, + path: ['customSeparators'], + }) + .refine( + (data) => + parseSeparators(data.customSeparators).every( + (separator) => separator.length <= MAX_CHUNKING_SEPARATOR_LENGTH + ), + { + message: `Each separator must be ${MAX_CHUNKING_SEPARATOR_LENGTH} characters or less`, + path: ['customSeparators'], + } + ) type FormInputValues = z.input type FormValues = z.output @@ -265,11 +288,7 @@ export const CreateBaseModal = memo(function CreateBaseModal({ ...(data.regexStrictBoundaries && { strictBoundaries: true }), } : data.strategy === 'recursive' && data.customSeparators?.trim() - ? { - separators: data.customSeparators - .split(',') - .map((s) => s.trim().replace(/\\n/g, '\n').replace(/\\t/g, '\t')), - } + ? { separators: parseSeparators(data.customSeparators) } : undefined const newKnowledgeBase = await createKnowledgeBaseMutation.mutateAsync({ @@ -465,11 +484,13 @@ export const CreateBaseModal = memo(function CreateBaseModal({ diff --git a/apps/sim/lib/api/contracts/knowledge/base.test.ts b/apps/sim/lib/api/contracts/knowledge/base.test.ts new file mode 100644 index 00000000000..6d9034bb6c6 --- /dev/null +++ b/apps/sim/lib/api/contracts/knowledge/base.test.ts @@ -0,0 +1,77 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + chunkingStrategyOptionsSchema, + createKnowledgeBaseBodySchema, + knowledgeBaseDataSchema, +} from '@/lib/api/contracts/knowledge/base' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from '@/lib/chunkers/constants' + +const separators = (count: number) => Array.from({ length: count }, (_, i) => `@@sep${i}@@`) + +describe('chunkingStrategyOptionsSchema.separators', () => { + it('accepts a separator list at the bound', () => { + const parsed = chunkingStrategyOptionsSchema.parse({ + separators: separators(MAX_CHUNKING_SEPARATORS), + }) + expect(parsed.separators).toHaveLength(MAX_CHUNKING_SEPARATORS) + }) + + it('rejects more separators than the bound', () => { + const result = chunkingStrategyOptionsSchema.safeParse({ + separators: separators(MAX_CHUNKING_SEPARATORS + 1), + }) + expect(result.success).toBe(false) + }) + + it('rejects a separator longer than the per-item bound', () => { + const result = chunkingStrategyOptionsSchema.safeParse({ + separators: ['|'.repeat(MAX_CHUNKING_SEPARATOR_LENGTH + 1)], + }) + expect(result.success).toBe(false) + }) + + it('rejects an oversized list on the knowledge base create body', () => { + const result = createKnowledgeBaseBodySchema.safeParse({ + name: 'kb', + workspaceId: 'ws', + chunkingConfig: { + maxSize: 1024, + minSize: 100, + overlap: 200, + strategy: 'recursive', + strategyOptions: { separators: separators(5000) }, + }, + }) + expect(result.success).toBe(false) + }) +}) + +describe('knowledgeBaseDataSchema.chunkingConfig', () => { + it('still reads a stored config written before the separator bound', () => { + const result = knowledgeBaseDataSchema.safeParse({ + id: 'kb-1', + userId: 'u-1', + name: 'kb', + description: null, + tokenCount: 0, + embeddingModel: 'text-embedding-3-small', + embeddingDimension: 1536, + chunkingConfig: { + maxSize: 1024, + minSize: 100, + overlap: 200, + strategy: 'recursive', + strategyOptions: { separators: separators(5000) }, + }, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + deletedAt: null, + workspaceId: 'ws', + folderId: null, + }) + expect(result.success).toBe(true) + }) +}) diff --git a/apps/sim/lib/api/contracts/knowledge/base.ts b/apps/sim/lib/api/contracts/knowledge/base.ts index 475c31e737e..541ec835c95 100644 --- a/apps/sim/lib/api/contracts/knowledge/base.ts +++ b/apps/sim/lib/api/contracts/knowledge/base.ts @@ -6,6 +6,7 @@ import { wireDateSchema, } from '@/lib/api/contracts/knowledge/shared' import { defineRouteContract } from '@/lib/api/contracts/types' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from '@/lib/chunkers/constants' import type { StrategyOptions } from '@/lib/chunkers/types' import { DEFAULT_CHUNKING_CONFIG, @@ -20,7 +21,13 @@ export const listKnowledgeBasesQuerySchema = z.object({ scope: knowledgeScopeSchema.default('active'), }) -export const chunkingStrategyOptionsSchema = z +/** + * Strategy options as they are stored. Reads stay tolerant of a `separators` + * list written before {@link chunkingStrategyOptionsSchema} bounded it, so an + * oversized legacy config lists instead of failing response validation. The + * chunker clamps such a list at construction, so nothing reprocesses unbounded. + */ +export const storedChunkingStrategyOptionsSchema = z .object({ pattern: z .string() @@ -42,6 +49,29 @@ export const chunkingStrategyOptionsSchema = z }) .strict() satisfies z.ZodType +/** + * Strategy options accepted on writes. `separators` is bounded in both length + * and item size: the recursive chunker rescans the whole document once per + * separator, synchronously, so an unbounded list turns one persisted config + * into seconds of uninterruptible CPU on every later document upload. + */ +export const chunkingStrategyOptionsSchema = storedChunkingStrategyOptionsSchema + .extend({ + separators: z + .array( + z + .string() + .max( + MAX_CHUNKING_SEPARATOR_LENGTH, + `Each separator must be ${MAX_CHUNKING_SEPARATOR_LENGTH} characters or less` + ) + ) + .max(MAX_CHUNKING_SEPARATORS, `At most ${MAX_CHUNKING_SEPARATORS} separators are allowed`) + .optional() + .describe('Ordered separators used to split content into chunks.'), + }) + .strict() satisfies z.ZodType + export const chunkingConfigSchema = z .object({ maxSize: z.number().min(100).max(4000), @@ -110,7 +140,7 @@ const knowledgeChunkingConfigSchema = z minSize: z.number(), overlap: z.number(), strategy: z.enum(['auto', 'text', 'regex', 'recursive', 'sentence', 'token']).optional(), - strategyOptions: chunkingStrategyOptionsSchema.optional(), + strategyOptions: storedChunkingStrategyOptionsSchema.optional(), }) .passthrough() diff --git a/apps/sim/lib/chunkers/constants.ts b/apps/sim/lib/chunkers/constants.ts new file mode 100644 index 00000000000..0e67a927407 --- /dev/null +++ b/apps/sim/lib/chunkers/constants.ts @@ -0,0 +1,17 @@ +/** + * Bounds on the separator list a recursive chunking config may carry. + * + * `RecursiveChunker` scans the whole document once per separator and walks the + * list from the top for every oversized fragment, so the separator count is a + * direct multiplier on synchronous CPU per document. The work happens inside a + * split loop, which neither the processing `Promise.race` timeout nor the + * after-the-fact chunk-count cap can interrupt — the list has to be bounded on + * the way in instead. + * + * The largest built-in recipe (`markdown`) uses 16 separators, so 32 leaves room + * for a hand-tuned list without letting one config stall the processing tier. + */ +export const MAX_CHUNKING_SEPARATORS = 32 + +/** Max characters in a single chunking separator. Real delimiters are a few characters. */ +export const MAX_CHUNKING_SEPARATOR_LENGTH = 100 diff --git a/apps/sim/lib/chunkers/recursive-chunker.test.ts b/apps/sim/lib/chunkers/recursive-chunker.test.ts index 345da36aaf3..7290f81a3b6 100644 --- a/apps/sim/lib/chunkers/recursive-chunker.test.ts +++ b/apps/sim/lib/chunkers/recursive-chunker.test.ts @@ -3,6 +3,7 @@ */ import { describe, expect, it } from 'vitest' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from './constants' import { RecursiveChunker } from './recursive-chunker' describe('RecursiveChunker', () => { @@ -101,6 +102,63 @@ describe('RecursiveChunker', () => { }) }) + describe('separator bounds', () => { + it.concurrent('ignores separators past the list bound', async () => { + const separators = [ + ...Array.from({ length: MAX_CHUNKING_SEPARATORS }, (_, i) => `@@nomatch${i}@@`), + '---', + ] + const chunker = new RecursiveChunker({ chunkSize: 15, separators }) + const text = + 'Section one content here with words.---Section two content here with words.---Section three content here.' + + const chunks = await chunker.chunk(text) + + expect(chunks.length).toBeGreaterThan(1) + expect(chunks.some((chunk) => chunk.text.includes('---'))).toBe(true) + }) + + it.concurrent('splits on a separator that survives the clamp', async () => { + const separators = [ + '---', + ...Array.from({ length: MAX_CHUNKING_SEPARATORS }, (_, i) => `@@nomatch${i}@@`), + ] + const chunker = new RecursiveChunker({ chunkSize: 15, separators }) + const text = + 'Section one content here with words.---Section two content here with words.---Section three content here.' + + const chunks = await chunker.chunk(text) + + expect(chunks.length).toBeGreaterThan(1) + expect(chunks.every((chunk) => !chunk.text.includes('---'))).toBe(true) + }) + + it.concurrent('drops a separator longer than the per-item bound', async () => { + const oversized = '|'.repeat(MAX_CHUNKING_SEPARATOR_LENGTH + 1) + const chunker = new RecursiveChunker({ chunkSize: 15, separators: [oversized, '---'] }) + const text = `Section one content here.${oversized}Section two content.---Section three content.` + + const chunks = await chunker.chunk(text) + + expect(chunks.some((chunk) => chunk.text.includes('|'))).toBe(true) + expect(chunks.every((chunk) => !chunk.text.includes('---'))).toBe(true) + }) + + it.concurrent('falls back to the recipe when every separator is over the bound', async () => { + const oversized = '|'.repeat(MAX_CHUNKING_SEPARATOR_LENGTH + 1) + const text = + 'Section one content here with words.\n\nSection two content here with words.\n\nSection three content.' + + const chunks = await new RecursiveChunker({ chunkSize: 15, separators: [oversized] }).chunk( + text + ) + const defaultChunks = await new RecursiveChunker({ chunkSize: 15 }).chunk(text) + + expect(chunks.length).toBeGreaterThan(1) + expect(chunks).toEqual(defaultChunks) + }) + }) + describe('recipe: plain', () => { it.concurrent('should use plain recipe by default', async () => { const chunker = new RecursiveChunker({ chunkSize: 20 }) diff --git a/apps/sim/lib/chunkers/recursive-chunker.ts b/apps/sim/lib/chunkers/recursive-chunker.ts index 0dba2240987..c60933787a6 100644 --- a/apps/sim/lib/chunkers/recursive-chunker.ts +++ b/apps/sim/lib/chunkers/recursive-chunker.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from '@/lib/chunkers/constants' import type { Chunk, RecursiveChunkerOptions } from '@/lib/chunkers/types' import { addOverlap, @@ -62,8 +63,29 @@ export class RecursiveChunker { this.chunkSize = resolved.chunkSize this.chunkOverlap = resolved.chunkOverlap - if (options.separators && options.separators.length > 0) { - this.separators = options.separators + /** + * Bounded here as well as at the API boundary: a config persisted before the + * boundary bound existed would otherwise still cost one full document scan + * per separator, synchronously, on every document it processes. + * + * An over-long separator is dropped rather than truncated — a truncated + * separator matches where the configured one never did, silently re-cutting + * the document, whereas dropping it behaves like a separator that finds no + * match, which the split already handles. + */ + const requested = options.separators ?? [] + const usable = requested + .filter((separator) => separator.length <= MAX_CHUNKING_SEPARATOR_LENGTH) + .slice(0, MAX_CHUNKING_SEPARATORS) + + if (usable.length < requested.length) { + logger.warn( + `Chunking config carries ${requested.length} separators; using ${usable.length} within the ${MAX_CHUNKING_SEPARATORS} × ${MAX_CHUNKING_SEPARATOR_LENGTH}-character bound` + ) + } + + if (usable.length > 0) { + this.separators = usable } else { const recipe = options.recipe ?? 'plain' this.separators = [...RECIPES[recipe]] @@ -77,21 +99,34 @@ export class RecursiveChunker { return text.trim() ? [text] : [] } - if (separatorIndex >= this.separators.length) { - const chunkSizeChars = tokensToChars(this.chunkSize) - return splitAtWordBoundaries(text, chunkSizeChars) - } + /** + * Advance past separators that do not split this text. Iterating rather + * than recursing keeps stack depth independent of the separator count. + */ + let index = separatorIndex + let separator = '' + let parts: string[] = [] - const separator = this.separators[separatorIndex] + while (index < this.separators.length) { + separator = this.separators[index] - if (separator === '') { - return this.splitRecursively(text, this.separators.length) - } + if (separator === '') { + index = this.separators.length + break + } - const parts = text.split(separator).filter((part) => part.trim()) + parts = text.split(separator).filter((part) => part.trim()) - if (parts.length <= 1) { - return this.splitRecursively(text, separatorIndex + 1) + if (parts.length > 1) { + break + } + + index++ + } + + if (index >= this.separators.length) { + const chunkSizeChars = tokensToChars(this.chunkSize) + return splitAtWordBoundaries(text, chunkSizeChars) } const chunks: string[] = [] @@ -108,7 +143,7 @@ export class RecursiveChunker { } if (estimateTokens(part) > this.chunkSize) { - const subChunks = this.splitRecursively(part, separatorIndex + 1) + const subChunks = this.splitRecursively(part, index + 1) for (const subChunk of subChunks) { chunks.push(subChunk) } From 6f94d58c2e64a632a1a9649c583c9c6982d526b9 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 15 Aug 2026 11:59:34 -0700 Subject: [PATCH 2/2] fix(knowledge): gate separator validation on the recursive strategy - The separator refines ran for every strategy, but the field only renders for `recursive` and only that strategy submits it, so a value left behind by a strategy switch could block submit with no visible field to clear. Gated the same way the regex-pattern refine already is. - Use absolute imports in the chunker test, per the repo convention. --- .../create-base-modal/create-base-modal.tsx | 19 +++++++++++++++---- .../lib/chunkers/recursive-chunker.test.ts | 4 ++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx b/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx index bf0b1c349f7..9722c695371 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx @@ -133,12 +133,23 @@ const FormSchema = z path: ['regexPattern'], } ) - .refine((data) => parseSeparators(data.customSeparators).length <= MAX_CHUNKING_SEPARATORS, { - message: `At most ${MAX_CHUNKING_SEPARATORS} separators are allowed`, - path: ['customSeparators'], - }) + /** + * Gated on the strategy for the same reason the regex pattern is: the field only + * renders for `recursive` and only that strategy submits it, so an out-of-bound + * value left behind by a strategy switch must not block a submit that drops it. + */ + .refine( + (data) => + data.strategy !== 'recursive' || + parseSeparators(data.customSeparators).length <= MAX_CHUNKING_SEPARATORS, + { + message: `At most ${MAX_CHUNKING_SEPARATORS} separators are allowed`, + path: ['customSeparators'], + } + ) .refine( (data) => + data.strategy !== 'recursive' || parseSeparators(data.customSeparators).every( (separator) => separator.length <= MAX_CHUNKING_SEPARATOR_LENGTH ), diff --git a/apps/sim/lib/chunkers/recursive-chunker.test.ts b/apps/sim/lib/chunkers/recursive-chunker.test.ts index 7290f81a3b6..441666ad5d3 100644 --- a/apps/sim/lib/chunkers/recursive-chunker.test.ts +++ b/apps/sim/lib/chunkers/recursive-chunker.test.ts @@ -3,8 +3,8 @@ */ import { describe, expect, it } from 'vitest' -import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from './constants' -import { RecursiveChunker } from './recursive-chunker' +import { MAX_CHUNKING_SEPARATOR_LENGTH, MAX_CHUNKING_SEPARATORS } from '@/lib/chunkers/constants' +import { RecursiveChunker } from '@/lib/chunkers/recursive-chunker' describe('RecursiveChunker', () => { describe('empty and whitespace input', () => {