Skip to content

fork: llama-engine /v1/chat/completions (hydra_vortex #356)#15

Merged
ddvnguyen merged 1 commit into
hydra-forkfrom
fork/hydra-356-engine-chat-completions
Jun 26, 2026
Merged

fork: llama-engine /v1/chat/completions (hydra_vortex #356)#15
ddvnguyen merged 1 commit into
hydra-forkfrom
fork/hydra-356-engine-chat-completions

Conversation

@ddvnguyen

Copy link
Copy Markdown
Owner

Implements the C++ side of ddvnguyen/hydra_vortex#356.

What & why

llama-engine (the hydra-fork ddvnguyen/llama.cpp variant under
tools/llama-engine/) never registered /v1/chat/completions. Hydra.Core's
HTTP proxy (CompletionProxyService) has routed all decode — engine and
legacy — through the OpenAI-schema POST /v1/chat/completions path since
PR ggml-org#273 / commit e17a6e8. Engine-mode requests 404 on the worker, the
Core's catch-all maps that to 503, and every decode fails. The misleading
comment in WorkerSchedulerService.cs:1670-1677 ("HTTP streaming for chat
completions works for both engine and legacy modes") becomes accurate
once the route exists.

How

Mirrors what tools/server/server.cpp already does. No C# change needed.

  1. Copy ex_wrapper (a static helper in server.cpp, ~L41-72) into
    llama-engine.cpp. It bridges the streaming
    std::unique_ptr<server_res_generator> handler signature returned by
    server_routes::post_* to the std::unique_ptr<server_http_res>
    signature expected by server_http_context::handler_t, and converts
    exceptions to JSON error responses.
  2. Construct server_routes routes(params, ctx_server); before
    server_http_context ctx_http; so the captured-lambda handlers stored
    in ctx_http are destroyed before routes (reverse declaration
    order would leave dangling captures).
  3. Inside the if (params.port > 0) block, register:
    ctx_http.post("/v1/chat/completions", ex_wrapper(routes.post_chat_completions));
    ctx_http.post("/chat/completions",    ex_wrapper(routes.post_chat_completions));
    ctx_http.post("/v1/completions",      ex_wrapper(routes.post_completions_oai));
  4. Call routes.update_meta(ctx_server); before
    ctx_http.is_ready.store(true);post_chat_completions reads
    meta->chat_params and would see a default (empty) chat template
    otherwise.

Total: +68 lines, all in tools/llama-engine/llama-engine.cpp. No header
or build-system changes — llama-engine already links server-context
(see tools/llama-engine/CMakeLists.txt), and server_routes is
already declared in server-context.h.

Out of scope (deferred)

Routing /v1/embeddings, /v1/responses, /v1/messages (Anthropic),
/v1/rerank, /tokenize, /detokenize, /apply-template, etc.
through server_routes would be mechanical (ex_wrapper(routes.X) for
each) but is a larger blast radius. Defer as a follow-up — full
llama-server parity for the engine binary isn't needed to unblock
Hydra's decode path.

Test plan

  • clang -fsyntax-only (clean)
  • cmake --build build_sm120 --target llama-engine (clean, sm_120)
  • cmake --build build_sm60 --target llama-engine (P100, CUDA 12.9)
  • Local: curl -X POST localhost:8080/v1/chat/completions -d '{\"messages\":[{\"role\":\"user\",\"content\":\"hi\"}],\"max_tokens\":4}'
    → 200 OpenAI JSON, with reasoning_content/finish_reason/timings
  • Repeat with "stream":true → SSE data: chunks + [DONE]
  • E2E on Hydra: HYDRA_LLAMA_ENGINE=true, POST localhost:9000/v1/chat/completions
    → 200 (was 503), no decode_crashed/404 in worker logs

The local -syntax-only and full build_sm120 passes were done
during development. Live HTTP smoke test was deferred to the Hydra-side
deploy step (production engine uses GPU memory that the new shared-library
binary is unable to fit alongside; deploying to a hydra-head-rtx restart
window is the appropriate verification point).

… (hydra_vortex ggml-org#356)

Hydra.Core's HTTP proxy (CompletionProxyService) POSTs /v1/chat/completions
on every worker since ggml-org#273 routed all decode — engine and legacy — through
the OpenAI-schema HTTP path. llama-engine never registered the route, so
engine-mode requests 404'd on the worker and were mapped to 503 by the
Core's catch-all.

This commit wires the existing server_routes handlers (post_chat_completions,
post_completions_oai) into llama-engine's HTTP server, mirroring what
tools/server/server.cpp already does:

- Copy ex_wrapper (static helper in server.cpp) into llama-engine.cpp
  to bridge the streaming handler signature to the response-object one
  expected by server_http_context::handler_t.
- Construct server_routes routes(params, ctx_server) BEFORE
  server_http_context ctx_http so the captured-lambda handlers stored in
  ctx_http are destroyed before routes (reverse declaration order would
  leave dangling captures).
- Register /v1/chat/completions, /chat/completions, /v1/completions.
- Call routes.update_meta(ctx_server) before flipping ctx_http.is_ready —
  the chat handler reads meta->chat_params and would see a default
  (empty) chat template otherwise.

No C# change needed; the WorkerSchedulerService.cs:1670-1677 comment about
HTTP streaming for chat completions working in both engine and legacy
modes becomes accurate once this lands.

Implements the C++ side of ddvnguyen/hydra_vortex#356.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@ddvnguyen ddvnguyen left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review of the C++ side of hydra_vortex#356 (this PR is already merged; flagging for follow-up). The route registration itself is sound and mirrors server.cpp. Two inline findings — one misleading lifetime comment, and one pre-existing shutdown defect (unjoined HTTP thread → std::terminate()) that this PR's lifetime reliance makes worth addressing.


Generated by Claude Code

Comment on lines +354 to +361
// Hydra #356: server_routes owns the schema-correct handlers
// (post_chat_completions, post_completions_oai) and the chat-template
// meta (read by oaicompat_chat_params_parse). Declared BEFORE
// server_http_context ctx_http so the captured-lambda handlers stored
// in ctx_http are destroyed before `routes` is destroyed — reverse
// declaration order would leave dangling captures in ctx_http. Mirrors
// the lifetime layout in tools/server/server.cpp (the routes struct is
// constructed in the same scope as ctx_http and used until shutdown).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This comment is factually inverted. It claims declaring routes before ctx_http "Mirrors the lifetime layout in tools/server/server.cpp" and that "reverse declaration order would leave dangling captures" — but server.cpp does the opposite: ctx_http is declared first (server.cpp:122), then routes (server.cpp:133).

The "dangling captures" rationale is also incorrect: destroying a std::function that captured thisroutes never dereferences routes, so the declaration order is harmless either way. (The real teardown hazard is the unjoined HTTP thread — see the shutdown comment.)

The chosen ordering is fine to keep; just fix the justification so it doesn't mislead. Suggest something like: "Declared before ctx_http so routes outlives the handlers ctx_http stores (copies of routes.post_*)."


Generated by Claude Code

// meta->chat_params, which is only populated by update_meta() —
// must run before ctx_http.is_ready flips or the first chat
// request will see a default-constructed (empty) chat template.
routes.update_meta(ctx_server);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Adjacent issue worth fixing while you're in this teardown region: llama_engine() never stops/joins the HTTP server thread on shutdown.

server_http_context holds a std::thread thread member (server-http.h:68) with a defaulted destructor. server.cpp joins it via clean_up()ctx_http.stop() + ctx_http.thread.join() (server.cpp:283-285, 370-372). Here, after ctx_server.start_loop() returns, the function only calls ctx_server.terminate() / llama_backend_free() and returns — the HTTP thread is still blocked in listen_after_bind(), so destroying the still-joinable thread member at scope exit triggers std::terminate()/SIGABRT.

On a hydra-head restart (SIGTERM → shutdown_handlerctx_server.terminate() unblocks start_loop() → return), the engine aborts instead of exiting cleanly, which can trip crash-loop / non-zero-exit alerting. Pre-existing, but this PR newly depends on the routes/ctx_http lifetime, so a ctx_http.stop() + join before return belongs here.


Generated by Claude Code

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