fix(knowledge): parse the stored artifact, not the document's display name - #6817
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Parser selection adds Connector sync storage centralizes stored object naming via Sync duration and stale locks move from scattered constants to Reviewed by Cursor Bugbot for commit c84d216. Configure here. |
Greptile SummaryThe PR makes parser selection reflect the stored artifact rather than a connector document’s display filename and raises the connector-sync runtime ceiling while preserving the stale-lock safety margin.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/knowledge/documents/parser-extension.ts | Adds guarded storage-key extension resolution while retaining the existing filename/MIME fallback. |
| apps/sim/lib/knowledge/documents/document-processor.ts | Prefers the downloaded internal artifact’s supported extension when selecting the non-OCR parser. |
| apps/sim/lib/knowledge/connectors/sync-engine.ts | Centralizes connector artifact naming so extracted text is consistently stored and processed as .txt. |
| apps/sim/lib/knowledge/connectors/sync-limits.ts | Defines a one-hour sync ceiling and derives the two-hour stale-lock TTL from it. |
| apps/sim/app/api/knowledge/connectors/sync/route.ts | Uses the shared stale-lock TTL when reclaiming interrupted connector syncs. |
| apps/sim/background/knowledge-connector-sync.ts | Raises the Trigger.dev connector-sync duration from 30 to 60 minutes through the shared constant. |
| apps/sim/lib/knowledge/documents/stored-artifact-extension.test.ts | Covers connector, upload, provider-prefix, external URL, unsupported suffix, and case-normalization behavior. |
| apps/sim/lib/knowledge/connectors/sync-limits.test.ts | Pins both the increased runtime and the minimum two-times stale-lock margin. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Connector[Connector extracts source content] --> TextObject[Store extracted text under .txt key]
Upload[Direct file upload] --> BinaryObject[Store original bytes under original extension]
TextObject --> Processor[Document processor downloads artifact]
BinaryObject --> Processor
Processor --> Internal{Internal storage URL?}
Internal -->|Yes| StoredExt[Resolve supported extension from storage key]
Internal -->|No or unresolved| Fallback[Resolve from display filename and MIME]
StoredExt --> Parser[Select parser]
Fallback --> Parser
Parser --> Chunks[Chunk and embed parsed content]
Reviews (2): Last reviewed commit: "fix(knowledge): raise the connector sync..." | Re-trigger Greptile
… name A connector document's `filename` is a display name that deliberately disagrees with the bytes on disk: the sync engine records the source file's name (`Report.pdf`) while storing the text the connector already extracted from it under a `.txt` key, with `mimeType: 'text/plain'`. `processDocumentAsync` discards the processing filename the sync engine computes and rebuilds its input from the document row, so the parser was chosen from the display name and re-parsed extracted text as the source binary. In production that failed 1,379 SharePoint PDFs with `Invalid PDF structure.` and silently double-wrapped 364 spreadsheets — those reported `completed`, wrapping a second fake sheet around the connector's own extraction, because SheetJS accepts almost any input. Parser selection now prefers the extension of the object actually fetched, falling back to the filename/MIME path when the URL is not ours or the key carries no extension a parser claims. Both ingestion paths are honest under that rule because `fitStorageKeyName` preserves extensions through truncation: an upload keys on its original name, a connector document keys on what it stored. This layer is what covers the stuck-document retry sweep, which rebuilds its own input from the same display name — the sweep is the path that reprocesses the already-failed documents, so a fix confined to `processDocumentAsync` would have left the remediation itself broken. The defect predates the connectors that expose it: Box fetches Box-side text representations for `pdf`/`docx`/`xlsx` and stores them under the source name too, so it was latent there before SharePoint and OneDrive reached binary formats. `connectorArtifactFileName` now owns the `.txt` suffix that the parser choice depends on, so the invariant is structural instead of a convention repeated at four call sites per function.
…ale lock A 2,600-document library exhausted the 30-minute budget and the run was killed mid-listing, leaving the connector's `syncing` lock set until the scheduler reclaimed it. Raising the ceiling is not a lone constant, because reclaiming a stale lock flips the connector to `error` and frees it for another sync. A TTL at or below the run ceiling would hand the lock to a successor while the first sync is still writing — two syncs racing the same `(connectorId, externalId)` rows. The previous values, a 1800s run against a hard-coded 120-minute TTL declared in a different file, held that invariant only by coincidence. Both now derive from one another, with a test pinning the margin so the next raise cannot silently break it.
dd343a1 to
c84d216
Compare
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c84d216. Configure here.
1 participant
Fixes a customer-reported SharePoint knowledge base in which 1,586 of 2,663 documents failed. Investigation found four distinct problems, plus 364 documents that reported success while holding corrupted content.
Root cause: connector content is parsed twice
A connector document's
filenameis a display name that deliberately disagrees with the bytes on disk.sync-engine.tsstores the text the connector already extracted under a.txtstorage key withmimeType: 'text/plain', while recordingfilename: 'Report.pdf'for the UI. It even computesprocessingFilename = ${safeTitle}.txtspecifically to avoid re-parsing.That filename never reaches the parser.
service.ts:863discards it and rebuilds its input from the document row:resolveParserExtensionprefers the filename extension over the MIME type, so already-extracted plain text is handed to the PDF parser.The outcome depends entirely on how tolerant that parser is — which is why this looked like "PDFs are broken" rather than what it is:
Invalid PDF structure.completedDocxParser's plaintext-fallback passes text throughThe corruption is visible in the stored chunks, SheetJS having wrapped the connector's own correct extraction in a second, fake sheet:
This predates the connectors that expose it. Box has always fetched Box-side text representations for
pdf/docx/xlsx/pptand stored them under the source filename, so it was latent there before SharePoint and OneDrive gained binary formats in #6785.The fix: parse what you fetched
resolveStoredArtifactExtension(fileUrl)reads the extension off the storage key — the object we actually downloaded — and parser selection prefers it, falling back to the existing filename/MIME path when the URL is not ours or the key has no extension a parser claims.Both paths are honest under this rule because
fitStorageKeyNamedeliberately preserves extensions through truncation:kb/<id>-Report.pdf→pdf(unchanged)kb/<id>-Report.pdf.txt→txt(correct — it is text)It can only ever redirect a document to a parser that exists, and it fixes the stuck-document retry sweep for free, since that builds its own input from the same display name.
PDF uploads are untouched: they carry
application/pdf, so they still short-circuit to Mistral OCR before reaching this code.Sync timeout: 30 → 60 minutes
The run for this KB was killed at the 30-minute cap mid-listing, leaving the connector's
syncinglock set until the scheduler reclaimed it.Raising it is not a one-line change, because reclaiming a stale lock flips the row to
errorand frees it for another sync. A TTL at or below the run ceiling would hand the lock to a successor while the first sync is still writing — two syncs racing on the same(connectorId, externalId)rows. The old values (1800s run vs a hard-coded 120-minute TTL) were safe only by coincidence.Both now derive from one another in
sync-limits.ts, with a test pinning the invariant so the next raise cannot silently break it. 60 minutes doubles capacity while keeping exactly the 2× margin that exists today.Not fixed here — needs a cost decision
Connector PDFs never reach OCR. The OCR branch is gated on
mimeType === 'application/pdf', and connector documents aretext/plain. Manual uploads go through Mistral OCR (confirmed on a live run:Using Mistral OCR); connector PDFs get localunpdf. Same file, two ingestion paths, different quality.That also leaves 137 genuinely scanned PDFs (9% of this customer's PDFs) unindexable — they have no text layer, so only OCR can read them. Notably the customer already pre-OCRs some files externally: of 92
*_ocr.pdf, 91 failed on this bug and only 1 truly lacked text.The real fix is to stop double-handling — have connectors store raw bytes with the true filename and MIME type and let the single existing pipeline parse them, which routes PDFs to OCR and makes this class of bug structurally impossible. Gated on OCR spend (~1,500 PDFs for this KB alone), storage cost, and reconciling
CONNECTOR_MAX_FILE_BYTES(100 MB/file) againstCONTENT_INFLIGHT_BUDGET_BYTES(64 MB total in flight).Cleanup of existing data
The stored
.txtcontent is correct — the damage happened at parse time, not at store time — so nothing needs re-downloading from SharePoint.failedwith a storage key inside the 7-day retry window, so the stuck-document sweep re-processes them from stored text on the next sync.completed, so the sweep skips them. They needretryDocumentProcessing(clears embeddings, resets topending, re-enqueues), which is the existing product primitive behind the retry action..docstay skipped with accurate reasons until the OCR decision above.Verification
vitest run lib/knowledge/ lib/uploads/ app/api/knowledge/ connectors/— 1,763 passed (115 files)tsgo --noEmit— clean apart from the pre-existing unrelatedmssqlbun run check:audits— 29/29