Skip to content

[codex] Fix Kreuzberg native runtime fallback - #2731

Closed
kwakayama wants to merge 1 commit into
mainfrom
codex/kreuzberg-native-runtime
Closed

kwakayama wants to merge 1 commit into
mainfrom
codex/kreuzberg-native-runtime

Conversation

@kwakayama

Copy link
Copy Markdown
Contributor

Summary

Fix the staging runtime failure found while proving veryfront-studio#5484:

  • bump veryfront to 0.1.990
  • keep the native Kreuzberg PDF/PPT progress path, but stop falling back to the OOM-prone Deno worker path when the native binding is missing
  • return a structured extraction failure instead of letting a large PDF push the server pod into OOMKilled/HTTP 502
  • add regression tests for missing native bindings with and without progress extraction

Root Cause

The post-merge staging proof did not reach successful PDF extraction. The runtime pod logged that @kreuzberg/node could not load its platform native binding, fell back to the previous opaque extraction path, then Kubernetes OOMKilled the veryfront-server pod. The API only surfaced that process death as Runtime execution returned HTTP 502.

Verification

  • deno task test in extensions/ext-document-kreuzberg
  • deno fmt --check deno.json src/utils/version-constant.ts extensions/ext-document-kreuzberg/src/index.ts extensions/ext-document-kreuzberg/src/index.test.ts
  • git diff --check
  • pre-push hook with observability env unset: formatting, lint, typecheck, and unit tests: 2223 passed (19094 steps), 0 failed

Notes

This PR pairs with the veryfront-server PR that stages @kreuzberg/node-linux-x64-gnu into the server image so the native path is actually available in staging.

@kwakayama
kwakayama requested a review from kojiwakayama as a code owner July 2, 2026 20:48
Copilot AI review requested due to automatic review settings July 2, 2026 20:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Veryfront and adjusts the ext-document-kreuzberg Deno extraction behavior to avoid falling back to the OOM-prone worker/opaque extraction path when Kreuzberg native bindings are unavailable, adding regression tests to lock in the new failure mode.

Changes:

  • Bump veryfront version to 0.1.990 (and keep VERSION constant in sync).
  • In KreuzbergDocumentExtractor, throw a dedicated “native binding unavailable” error instead of falling back when native bindings are missing (both for PDF-native and native-progress paths).
  • Update/add tests to assert that missing native bindings fail fast and do not use the worker/opaque fallback.

Verification

  • Not run in this review environment. Based on PR notes, the author ran deno task test under extensions/ext-document-kreuzberg, deno fmt --check ..., and pre-push checks.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
deno.json Bumps package version to 0.1.990.
src/utils/version-constant.ts Keeps exported VERSION aligned with deno.json.
extensions/ext-document-kreuzberg/src/index.ts Stops Deno PDF/native-progress extraction from falling back when native bindings are missing; introduces a dedicated error wrapper.
extensions/ext-document-kreuzberg/src/index.test.ts Updates tests to assert “fail instead of fallback” behavior for missing native bindings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +117 to +122
function createNativeBindingUnavailableError(error: unknown): Error {
return new Error(
"Native document extraction is unavailable because the Kreuzberg native binding could not be loaded.",
{ cause: error },
);
}
@kwakayama
kwakayama force-pushed the codex/kreuzberg-native-runtime branch from 580d5c3 to edffee5 Compare July 2, 2026 21:00
@kwakayama

Copy link
Copy Markdown
Contributor Author

Closing this follow-up because we are keeping the no-progress PDF behavior that falls back to the Deno worker when native PDF extraction is unavailable. The deploy/runtime fix continues in veryfront-server#209, which ships the native binding in the server image.

@kwakayama kwakayama closed this Jul 2, 2026
@kwakayama
kwakayama deleted the codex/kreuzberg-native-runtime branch July 2, 2026 21:13
@kwakayama

Copy link
Copy Markdown
Contributor Author

Critical Review

Score: 68/100. Right fix, right direction: fail fast with a structured error beats an OOMKilled pod surfacing as an opaque HTTP 502. But the headline detection mechanism is dead code in production, and the new test proves a scenario that cannot happen.

1. The progress-path missing-binding check never fires in production (P1, main issue)

The chain, verified against actual sources:

  1. Binding missing inside the progress worker: extractPdfByPage calls loadKreuzbergNative(), and @kreuzberg/node throws "Cannot find native binding. npm has a bug..." (node_modules/@kreuzberg/node/index.js:564).
  2. loadKreuzbergNative (kreuzberg.ts:28) classifies it and rethrows as 'Native document extraction requires the optional package "@kreuzberg/node". Install @kreuzberg/node@^4.4.2 or disable document extraction.' with the original error as cause.
  3. The worker's catch (native-progress-extraction-worker.ts, onmessage) posts only error.message. The cause chain does not survive postMessage.
  4. Main thread (index.ts:242): isMissingPackageError on that wrapped message checks for "Cannot find package" / "Cannot find module" / "Cannot find native binding" / "ERR_MODULE_NOT_FOUND" / "Module not found". None match. Returns false.

So the new throw createNativeBindingUnavailableError(error) in the progress-path catch is unreachable for the real error. Actual production flow: warn "falling back to opaque extraction", fall through to the opaque PDF path, and only there (index.ts:266, where the cause chain is intact in-process) produce the structured error.

Net behavior for PDFs is still correct, which is why this scores 68 and not 40. But:

  • The new test (index.test.ts:329) hand-crafts "Failed to load Kreuzberg native bindings. Underlying error: Cannot find native binding.", a string no production component emits across the worker boundary. The test proves a fiction and gives false confidence in the exact mechanism this PR exists to add.
  • Ops impact: Grafana shows a warn promising "falling back to opaque extraction" immediately followed by a hard failure for the same file. On-call chases a phantom "fallback also broke" hypothesis. warningDetails also drops error.cause, so the real missing-binding reason is absent from the log.

Fix: classify in the worker and send a structured flag across the boundary, e.g. the worker catch posts { type: "error", error: message, missingNativeBinding: isMissingPackageError(error) }, and extractWithNativeProgressDeno rethrows a typed error the main-thread catch can trust. String matching across a postMessage boundary is exactly how this broke; do not add another string.

2. isMissingPackageError misses the other napi failure mode (P3)

@kreuzberg/node also throws bare "Failed to load native binding" when loadErrors is empty (index.js:575). That matches no pattern in isMissingPackageError (kreuzberg.ts), so it escapes both loadKreuzbergNative's wrap and this PR's structured error, and surfaces raw. Add the pattern.

3. The throw is mime-agnostic but the rationale is PDF-specific (P2, latent)

The progress path serves PDF, PPT, and PPTX (isNativeProgressMimeType). Once finding 1 is fixed, PPT and slide-less PPTX on binding-less hosts flip from a working WASM fallback to hard failure, even though the OOM problem cited is the WASM PDF path. Either scope the throw to PDFs or state explicitly that PPT/PPTX fail-fast is intended.

4. Error message references a knob that does not exist (P2)

"...or disable document extraction for this file type" (index.ts:120): there is no per-type disable anywhere in the repo. No env var, no flag, no config. An operator reading this at 3am will grep for a setting that is not there. Say what is true: install the platform-specific Kreuzberg native package, and name it like loadKreuzbergNative does.

5. Deliberate regression, wider than the pod (informational)

Any Deno consumer of the npm package without @kreuzberg/node (unsupported platform, optional-dep install failure) loses PDF extraction entirely; small PDFs previously worked via the WASM worker. The paired veryfront-server PR covers staging, but rollout ordering matters: if veryfront-api bumps to 0.1.990 before the server image carries the binding, all PDF ingest fails. Fast and with a clear message, which is the point, but be ready for it.

What holds up

  • Root cause correctly identified; fail-fast is the right call for memory-limited pods.
  • The opaque-path restructure (index.ts:264-270) is correct and reachable. It is what actually delivers the structured error today.
  • Version bump pair (deno.json + version-constant.ts) follows the test-enforced convention.
  • Worker lifecycle is clean: workers terminated on all paths, buffer.slice(0) before transfer keeps the fallback buffer valid.
  • Copilot's earlier comment (generic error text) is already addressed at head.

Verdict: 68/100. Fix #1 (worker-side classification) and #4 (phantom knob) before merge; #2 and #3 are cheap to fold in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants