Skip to content

Fix onxy module Netty config#1532

Merged
alexheifetz merged 2 commits intomainfrom
bugfix/onyx-netty-redirects
Mar 31, 2026
Merged

Fix onxy module Netty config#1532
alexheifetz merged 2 commits intomainfrom
bugfix/onyx-netty-redirects

Conversation

@jasperblues
Copy link
Copy Markdown
Contributor

Fix ONNX model download failure in CI

Problem

The Export Seed Database workflow fails because OnnxModelLoader downloads truncated files from HuggingFace (1KB instead of ~30MB), causing an ORT_INVALID_PROTOBUF crash on startup. (Currently has workaround applied: Pre-download using curl)

Context

OnnxEmbeddingAutoConfiguration previously used the default JDK RestClient because Reactor Netty didn't follow redirects, and HuggingFace serves model files via 302 redirects to CDN URLs. Since then, NettyClientAutoConfiguration was updated with .followRedirect(true) — but the ONNX config was never updated to use it.

This worked locally because other starters on the classpath (e.g. OpenAI, Anthropic) pull in the netty module transitively. In CI — or any deployment where embabel-agent-starter-onnx is the only starter making HTTP downloads — the netty factory was absent, so RestClient fell back to the default JDK HttpURLConnection client, which doesn't follow HuggingFace's cross-origin CDN redirects.

Fix

Two changes to wire ONNX downloads through the shared netty-backed HTTP client:

  1. Add @Qualifier("aiModelHttpRequestFactory") to OnnxEmbeddingAutoConfiguration's requestFactory parameter — matching Anthropic, OpenAI, and Ollama configs. Without this, Spring may not inject the netty-backed factory even when it's available.
  2. Add embabel-agent-netty-client-autoconfigure dependency to embabel-agent-starter-onnx — ensures the netty module is on the classpath even when no other starter brings it in.

Copy link
Copy Markdown
Contributor

@alexheifetz alexheifetz left a comment

Choose a reason for hiding this comment

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

My only concern would be "baking" Netty into model starter will affect other components in the User App relying on generic ClientHttpRequestFactory (without user actually knowing that the "flip" to Netty had occurred)

Maybe better approach is to explicitly instruct Users to add Netty into the mix.

We can create a dedicated starter for it.

That way Users will be aware what is going on, and it will up to the them,
making sure Netty plays along with other application components that depend on ClientHttpRequestFactory

@jasperblues
Copy link
Copy Markdown
Contributor Author

jasperblues commented Mar 24, 2026

My only concern would be "baking" Netty into model starter will affect other components in the User App relying on generic ClientHttpRequestFactory (without user actually knowing that the "flip" to Netty had occurred)

Maybe better approach is to explicitly instruct Users to add Netty into the mix.

We can create a dedicated starter for it.

That way Users will be aware what is going on, and it will up to the them, making sure Netty plays along with other application components that depend on ClientHttpRequestFactory

Good point about Netty being pulled in transitively. That was actually the concern that led to the original design in #1478.

The initial implementation used a self-contained RestClient.create() (JDK HttpURLConnection-backed) for ONNX model downloads because it follows redirects natively, without requiring Netty or any shared HTTP factory.

This was deliberate because ONNX is the only provider that downloads large files from HuggingFace via 302
redirects to CDN URLs. That's a fundamentally different HTTP use case from the direct API calls made by
Anthropic/OpenAI/Ollama.

When this was later wired through the shared @Bean("aiModelHttpRequestFactory") for consistency with other providers, the qualifier was missed and the netty dependency wasn't added to the starter. So in environments where no other starter brings in Netty (like CI), the download silently falls back to a client that doesn't handle HuggingFace's cross-origin redirects.

I don't think it is a good solution to require users to add a separate Netty starter. It means the ONNX starter doesn't work out of the box. Users would hit a cryptic ORT_INVALID_PROTOBUF error with no indication that a missing HTTP dependency is the cause).

Recommendation

I think the better fix is to revert to the original approach: let OnnxModelLoader use its own JDK-backed RestClient for downloads.

Justification

  • Works out of the box with no extra dependencies
  • Doesn't pull Netty onto the user's classpath
  • Addresses the concern about affecting other components relying on ClientHttpRequestFactory

In this case, it is better for maintainers to absorb a minor asymmetry with other providers on behalf of users - in exchange for a friction-less experience that "just works"

Happy to update this PR accordingly.

@alexheifetz
Copy link
Copy Markdown
Contributor

alexheifetz commented Mar 28, 2026

Is it possible to set Rest Client as default, but still have an option for Netty, similar pattern as with other Providers?
That way if Users are interested in Netty that can drop it in and make a switch from their end.

Basically, I would keep both: default that works for ONYX (Rest Client if that works), and ability for users to drop in Netty Autoconfiguration if that is what they want and decide to proceed with.

@jasperblues
Copy link
Copy Markdown
Contributor Author

@alexheifetz Agreed on keeping Netty opt-in. Removed the hard dep from the ONNX starter. The fallback is now JdkClientHttpRequestFactory with HttpClient.Redirect.NORMAL, which handles the HuggingFace CDN redirects natively - no extra dependency needed. Netty is still picked up automatically when it's on the classpath.

…e fallback when aiModelHttpRequestFactory is absent, so CDN redirects are followed in ONNX-only projects. Netty is still used when present.
@jasperblues jasperblues force-pushed the bugfix/onyx-netty-redirects branch from 49d6cd8 to 4092abb Compare March 29, 2026 22:58
@sonarqubecloud
Copy link
Copy Markdown

@alexheifetz alexheifetz merged commit d32a707 into main Mar 31, 2026
17 checks passed
@alexheifetz alexheifetz deleted the bugfix/onyx-netty-redirects branch March 31, 2026 00:03
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