Skip to content

Commit 9bb9e72

Browse files
committed
fix(knowledge): bound the workbook preview to the rows it emits
`sheet_to_json` allocates from a worksheet's DECLARED `!ref` range rather than its populated cells, and Excel routinely writes an inflated range from stray formatting. The 1,000-row preview cap was applied to the result, so it bounded the emitted string while the allocation it was meant to bound had already happened. An 880 KB workbook exhausted an 8 GB worker; the same content exhausted 16 GB when this ran inside the connector sync. No machine size fixes that, because the allocation scales with a number the file declares about itself — fleet p99 for this task is 691 MB against 8 GB, so this is a cliff, not pressure. Passing the window into the conversion is what makes the cap real. `defval` goes with it: defaulting every cell in the range made each row dense, so allocation scaled with columns x declared rows rather than with populated cells, and because no row was left empty it silently defeated the `blankrows: false` beside it. Reported totals still come from the declared range, so bounding the conversion does not change what the metadata says the workbook holds. The eleven documents killed this way recorded `attempt_count = 1`: `maxAttempts` does not cover `TASK_PROCESS_OOM_KILLED`, which Trigger.dev retries only when a larger preset is named. Adding that escalation is a safety net rather than the fix, and the same gap the dispatcher had. Also corrects the machine comment, which claimed `large-1x` was 2 vCPU / 2 GB. It is 4 vCPU / 8 GB, and believing the stale figure makes a resize look like the answer when the parser is what is unbounded.
1 parent bfb6129 commit 9bb9e72

4 files changed

Lines changed: 121 additions & 6 deletions

File tree

apps/sim/background/knowledge-processing.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,17 @@ describe('knowledge processing worker', () => {
139139
)
140140
})
141141
})
142+
143+
describe('knowledge-process-document task configuration', () => {
144+
/**
145+
* `maxAttempts` does not cover an out-of-memory kill — Trigger.dev retries
146+
* `TASK_PROCESS_OOM_KILLED` only when a larger preset is named. Eleven
147+
* documents were killed in one afternoon and every one recorded
148+
* `attempt_count = 1`, so each was left `failed` having never been retried.
149+
*/
150+
it('escalates to a larger machine on an out-of-memory kill', async () => {
151+
const { processDocument } = await import('@/background/knowledge-processing')
152+
153+
expect(processDocument.retry?.outOfMemory?.machine).toBe('large-2x')
154+
})
155+
})

apps/sim/background/knowledge-processing.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,22 @@ export async function runDocumentProcessing(rawPayload: DocumentProcessingPayloa
5656
export const processDocument = task({
5757
id: 'knowledge-process-document',
5858
maxDuration: envNumber(env.KB_CONFIG_MAX_DURATION, 600),
59-
machine: 'large-1x', // 2 vCPU, 2GB RAM - needed for large PDF processing
59+
machine: 'large-1x', // 4 vCPU, 8GB RAM - needed for large PDF processing
6060
retry: {
6161
maxAttempts: envNumber(env.KB_CONFIG_MAX_ATTEMPTS, 3),
6262
factor: envNumber(env.KB_CONFIG_RETRY_FACTOR, 2),
6363
minTimeoutInMs: envNumber(env.KB_CONFIG_MIN_TIMEOUT, 1000),
6464
maxTimeoutInMs: envNumber(env.KB_CONFIG_MAX_TIMEOUT, 10000),
65+
/**
66+
* `maxAttempts` does not cover an out-of-memory kill — Trigger.dev retries
67+
* `TASK_PROCESS_OOM_KILLED` only when a larger preset is named here. Eleven
68+
* documents were killed in one afternoon and every one recorded
69+
* `attempt_count = 1`, so each was left `failed` with no retry at all. The
70+
* escalation is a safety net, not the fix: the workbook parser's allocation
71+
* no longer scales with a sheet's declared range, and fleet p99 memory is
72+
* 691 MB against this machine's 8 GB.
73+
*/
74+
outOfMemory: { machine: 'large-2x' },
6575
},
6676
queue: {
6777
concurrencyLimit: envNumber(env.KB_CONFIG_CONCURRENCY_LIMIT, 20),

apps/sim/lib/file-parsers/xlsx-parser.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,24 +86,45 @@ export class XlsxParser implements FileParser {
8686

8787
logger.info(`Processing sheet: ${sheetName} with ${rowCount} rows`)
8888

89-
// Convert to JSON with header row
89+
/**
90+
* Converted over a bounded window rather than the whole sheet.
91+
*
92+
* `sheet_to_json` allocates from the worksheet's DECLARED `!ref` range,
93+
* not from its populated cells, and Excel routinely writes an inflated
94+
* range from stray formatting — a sheet claiming hundreds of thousands of
95+
* rows materializes that many arrays whatever it actually contains. The
96+
* row cap below used to be applied after the conversion, so it bounded the
97+
* emitted string while the allocation it was meant to bound had already
98+
* happened: an 880 KB workbook exhausted an 8 GB worker, and the same
99+
* content exhausted 16 GB when this ran inside the connector sync. No
100+
* machine size fixes that, because the allocation scales with a number the
101+
* file declares about itself.
102+
*
103+
* `defval` is gone with it. Defaulting every cell in the range made each
104+
* row dense — allocation proportional to columns x rows rather than to
105+
* populated cells — and, because no row was left empty, it also silently
106+
* defeated the `blankrows: false` beside it.
107+
*/
108+
const lastPreviewRow = Math.min(range.e.r, range.s.r + CONFIG.MAX_PREVIEW_ROWS - 1)
90109
const sheetData = XLSX.utils.sheet_to_json(worksheet, {
91110
header: 1,
92-
defval: '', // Default value for empty cells
93111
blankrows: false, // Skip blank rows
112+
range: { s: { r: range.s.r, c: range.s.c }, e: { r: lastPreviewRow, c: range.e.c } },
94113
})
95114

115+
// Reported from the declared range, as before, so bounding the conversion
116+
// does not change what the metadata says the workbook holds.
96117
const actualRowCount = sheetData.length
97-
totalRows += actualRowCount
118+
totalRows += rowCount
98119

99120
// Store limited sample for metadata
100121
if (sampledData.length < CONFIG.MAX_SAMPLE_ROWS) {
101122
const sampleSize = Math.min(CONFIG.MAX_SAMPLE_ROWS - sampledData.length, actualRowCount)
102123
sampledData.push(...sheetData.slice(0, sampleSize))
103124
}
104125

105-
// Only process limited rows for preview
106-
const rowsToProcess = Math.min(actualRowCount, CONFIG.MAX_PREVIEW_ROWS)
126+
// Already bounded by the conversion window above.
127+
const rowsToProcess = actualRowCount
107128
const cleanSheetName = sanitizeTextForUTF8(sheetName)
108129

109130
// Add sheet header
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { afterEach, describe, expect, it, vi } from 'vitest'
5+
import * as XLSX from 'xlsx'
6+
import { XlsxParser } from '@/lib/file-parsers/xlsx-parser'
7+
8+
/**
9+
* A workbook holding three populated rows while declaring a range of 200,000 —
10+
* the shape Excel writes when stray formatting inflates `!ref`, and the shape
11+
* that exhausted an 8 GB worker.
12+
*/
13+
function inflatedRangeWorkbook(): Buffer {
14+
const sheet = XLSX.utils.aoa_to_sheet([
15+
['header-a', 'header-b'],
16+
['row-1-a', 'row-1-b'],
17+
['row-2-a', 'row-2-b'],
18+
])
19+
sheet['!ref'] = 'A1:B200000'
20+
const book = XLSX.utils.book_new()
21+
XLSX.utils.book_append_sheet(book, sheet, 'Sheet1')
22+
return XLSX.write(book, { type: 'buffer', bookType: 'xlsx' }) as Buffer
23+
}
24+
25+
describe('XlsxParser preview bound', () => {
26+
afterEach(() => {
27+
vi.restoreAllMocks()
28+
})
29+
30+
it('converts only the preview window, not the declared range', async () => {
31+
const toJson = vi.spyOn(XLSX.utils, 'sheet_to_json')
32+
33+
await new XlsxParser().parseBuffer(inflatedRangeWorkbook())
34+
35+
expect(toJson).toHaveBeenCalled()
36+
const options = toJson.mock.calls[0][1] as {
37+
range?: { s: { r: number }; e: { r: number } }
38+
defval?: unknown
39+
}
40+
41+
/**
42+
* The row cap used to be applied AFTER conversion, so it bounded the emitted
43+
* string while the allocation it was meant to bound had already happened.
44+
* Passing the window into the conversion is what makes the cap real.
45+
*/
46+
expect(options.range).toBeDefined()
47+
const rowsRequested =
48+
(options.range as { s: { r: number }; e: { r: number } }).e.r -
49+
(options.range as { s: { r: number }; e: { r: number } }).s.r +
50+
1
51+
expect(rowsRequested).toBeLessThanOrEqual(1000)
52+
53+
/**
54+
* `defval` made every cell in the range materialize, so allocation scaled
55+
* with columns x declared rows — and, because no row was left empty, it
56+
* silently defeated the `blankrows: false` sitting beside it.
57+
*/
58+
expect(options.defval).toBeUndefined()
59+
})
60+
61+
it('still reports the workbook the sheet declares', async () => {
62+
const result = await new XlsxParser().parseBuffer(inflatedRangeWorkbook())
63+
64+
// Bounding the conversion must not change what the metadata claims the
65+
// workbook holds, only how much of it is materialized to say so.
66+
expect(result.metadata?.totalRows).toBe(200000)
67+
expect(result.content).toContain('header-a')
68+
expect(result.content).toContain('row-2-b')
69+
})
70+
})

0 commit comments

Comments
 (0)