fork: join HTTP server thread on llama_engine() shutdown (hydra_vortex #360 review) - #18
Merged
Merged
Conversation
After `ctx_server.start_loop()` returns (unblocked by the signal-triggered `shutdown_handler` on SIGTERM/SIGINT), the HTTP server thread is still blocked in `pimpl->srv->listen_after_bind()` (server-http.cpp:385). `server_http_context::thread` is a `std::thread` with a defaulted destructor (server-http.h:68), so destroying the still-joinable thread at scope exit calls `std::terminate()` → SIGABRT. `tools/server/server.cpp:283-285, 369-372` already handles this via `clean_up()` → `ctx_http.stop()` + `ctx_http.thread.join()`. The engine predates that pattern; this PR brings it into line. Live-confirmed: podman logs of the production llama-engine on RTX showed `terminate called without an active exception` after the #15 deploy restarted it (the bug was latent — #15 just exposed it). With this fix, SIGTERM should drain the HTTP server, join the thread, and let the process exit 0 — matching `server.cpp`'s graceful-shutdown contract. While here, fix the misleading lifetime-ordering comment that #15 added at llama-engine.cpp:354-361 (#17): the chosen declaration order (`routes` before `ctx_http`) is fine because `ex_wrapper` captures `routes.post_*` by value, but the previous comment's "mirrors server.cpp" claim was the opposite of the actual layout (server.cpp:122 declares `ctx_http` first, `routes` at server.cpp:133), and the "dangling captures" rationale was wrong. Closes #16 Closes #17 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen
pushed a commit
to ddvnguyen/hydra_vortex
that referenced
this pull request
Jun 26, 2026
Fork PR: ddvnguyen/llama.cpp#18 (review follow-up to #15) Hydra issue: #356 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16
Closes #17
Follows up on #15 (which added /v1/chat/completions to
llama-engine; that PR is already merged into hydra-fork at 8f3dc58 and
bumped in ddvnguyen/hydra_vortex#360). The review on ggml-org#360 surfaced two
findings, both in
tools/llama-engine/llama-engine.cpp.Finding 1 (P1) —
std::terminate()on shutdownAfter
ctx_server.start_loop()returns (unblocked by the signal handleron SIGTERM/SIGINT — e.g. a hydra-head restart), the HTTP server thread is
still blocked in
pimpl->srv->listen_after_bind()(server-http.cpp:385).server_http_context::threadis astd::threadwith a defaulteddestructor (server-http.h:68), so destroying the still-joinable thread at
scope exit calls
std::terminate()→ SIGABRT. The hydra-headauto-restart logic masks it, but the process never exits cleanly and the
crash-loop alerting may fire.
Live-confirmed on the #15 production build: podman
logs of hydra-system_head-rtx_1 showed
terminate called without an active exceptionafter the deploy's auto-restart, with a backtrace through
~server_http_context→llama_engine→__libc_start_main.Fix
Mirrors
tools/server/server.cpp:283-285, 369-372exactly:The
if (params.port > 0)guard mirrors server.cpp's pattern of onlyrunning HTTP teardown when the HTTP server was actually started.
Finding 2 (P2) — lifetime-ordering comment was factually inverted
The comment added in #15 at llama-engine.cpp:354-361 claimed the chosen
routes-before-ctx_httpdeclaration order "Mirrors the lifetimelayout in tools/server/server.cpp" and that "reverse declaration order
would leave dangling captures". Both claims are wrong:
ex_wrapperreturns astd::functionthat capturesroutes.post_*by value; destroying it never dereferences
routes, so both ordersare safe. The real teardown hazard was the unjoined HTTP thread (Finding
1), not lambda captures.
The declaration order is fine — the comment now accurately describes
what it does and cross-references #16.
Verification
clang -fsyntax-onlycleancmake --build build_sm120 --target llama-engine(RTX, sm_120)showed the exact
std::terminate()from this issuecmake --build build_sm60 --target llama-engine(P100, sm_60)→ process exits 0, no SIGABRT, no zombie. Deferred to the
hydra-vortex deploy step (production engine uses 14.4 GB GPU and
the container's
/llama/binis read-only, so the new binarycan only land via an image rebuild — the right place to verify
end-to-end).
Cross-repo
will be force-pushed to this PR's merge SHA after it lands