fork: Unified RPC server — one port, one binary, no extra flags (closes #29)

[ddvnguyen]

Implements Phase A of #29.

Eliminates --peer-only, --ggml-rpc-port, --rpc-port, --combined-ot-pattern,
--combined-split-mode, --combined-tensor-split flags.

What changed

• transport.h/cpp: Added socket_t::from_fd(int fd) factory
• ggml-rpc.cpp: Added ggml_backend_rpc_handle_client() exported function
• server-context.h/cpp: Unified start_rpc_server with protocol detection
  (first byte 0x0E → ggml-RPC, else → Hydra protocol)
• llama-engine.cpp: Removed 5 flags, made --model optional, auto-derived
  RPC port, no-model path with ggml-RPC + minimal HTTP

New CLI

llama-engine --model m.gguf --port 8080                  # SOLO + auto RPC
llama-engine --model m.gguf --port 8080 --rpc-engine B:9504  # SOLO + COMBINE (testing)
llama-engine --port 8081                                  # compute-only backend

Build verified

• ggml-rpc: compiles clean
• llama-server (server-context.cpp): compiles clean
• llama-engine: compiles clean (zero warnings)

Cross-repo links

• Issue: Unified RPC Server: one port, one binary, no extra flags #29
• Hydra parent: [Llama-Engine] Unified RPC Server: per-request COMBINE + peer lifecycle hydra_vortex#392 (Phase D C# changes)

[ddvnguyen]

Code Review: PR #30 — Unified RPC Server

Overall: solid implementation that cleanly achieves the goal. The unified accept loop with MSG_PEEK protocol detection is elegant, the flag cleanup is thorough, and the no-model compute-only path is well-structured. A few issues to address before merge.

---

Critical (must fix)

1. No-model path: enumerate_non_cpu_devices() leaks backends

llama-engine.cpp — enumerate_non_cpu_devices() calls ggml_backend_dev_init(dev, nullptr) for each non-CPU device, returning ggml_backend_t objects. These are never freed. In the no-model path, backends is captured by value in the detached thread lambda, but the vector itself (and any ggml_backend_t objects not captured) leak.

Additionally, llama_backend_load_all() is called before llama_backend_init() in the has_peer branch (line ~255). The ggml docs say ggml_backend_load_all() should be called after llama_backend_init().

Fix: Call llama_backend_init() before ggml_backend_load_all(), and add cleanup for the enumerate_non_cpu_devices() result in the no-model path (or document that these are intentionally leaked for process lifetime).

---

2. g_current_peer static is not thread-safe

server-context.cpp line ~3555 — static std::string g_current_peer is accessed from the SET_EXPERT_MODE handler without synchronization. If two requests arrive concurrently (different slots), they race on this static. One request could call hydra_remove_combined_rpc_backend while another is mid-decode using the same peer backend.

Fix: Protect g_current_peer with a mutex, or move it to server_context_impl where it can be serialized through the task queue.

---

3. hydra_remove_combined_rpc_backend calls sched_reserve() mid-lifecycle

llama-context.cpp — hydra_remove_combined_rpc_backend calls build_backend_buffer_vectors() + sched_reserve() which destroys and recreates the scheduler. The comment at line 442 warns this is only safe "before the first decode." Calling this from a request handler while another slot may be decoding is unsafe — it invalidates the scheduler's internal hash tables, buffer allocations, and graph split state.

Fix: Either:

• (a) Add a guard: if (is_processing()) return false; — only remove when no slots are active, or
• (b) Defer the actual removal to a safe point (e.g., after the current batch completes), or
• (c) Document this as "caller must ensure no decode is in-flight" (the header comment already says this, but the handler doesn't check).

---

Medium (should fix)

4. const_cast in start_rpc_server and no-model path

server-context.cpp — const_cast<ggml_backend_t *>(backends.data()) appears twice. The ggml_backend_rpc_handle_client signature takes ggml_backend_t * (non-const), but the backends vector is const std::vector<ggml_backend *> &. This is a type-safety hole.

Fix: Change start_rpc_server to accept std::vector<ggml_backend_t> by value (or non-const ref) and own the data, or change the extern declaration to accept const ggml_backend_t *.

---

5. llama_hydra_clear_combined_bindings clears ALL layers unconditionally

llama-hydra.cpp line ~239 — the function iterates all layers and nulls ffn_*_exps_rpc for every layer, regardless of which endpoint the binding belongs to. If a future design supports multiple peers simultaneously, this would break.

Fix: Minor — acceptable for single-peer design. Add a comment: "clears all layers because the current design supports one peer at a time."

---

6. Forward declaration in server-context.cpp vs definition in ggml-rpc.cpp

server-context.cpp declares extern void ggml_backend_rpc_handle_client(...) and llama-engine.cpp does the same. These must exactly match the definition in ggml-rpc.cpp. If the signature changes in ggml-rpc.cpp, the extern declarations silently become ABI-incompatible.

Fix: Add the declarations to a shared header (e.g., ggml-rpc.h) instead of inline extern declarations. Or at minimum, add a static_assert or compile-time check.

---

Minor (nice to have)

7. Stripped flags are silently consumed

extract_hydra_capability_flags strips --ggml-rpc-port, --peer-only, --combined-ot-pattern, etc. without any log message. An operator who still has these in their config will see them silently ignored. A one-time warning would help debugging.

---

8. set_hydra_capabilities hardcoded to "solo" split mode

llama-engine.cpp line ~280 — ctx_server.set_hydra_capabilities(true, flags.rpc_engine_peer, false, "", "solo") hardcodes "solo" as the split mode. This means the INFO RPC (0x41) always reports "solo" even when --rpc-engine is set. The CombinedCapable field in the capabilities depends on hydra_combined_head_attached, which is now set based on the ggml_backend_rpc_add_server return value. Verify this chain works correctly — if the peer is reachable at startup, does CombinedCapable get reported as true?

---

9. No-model path doesn't register local tensors

The model-loaded path calls llama_hydra_register_local_tensors_for_rpc (line ~280). The no-model path doesn't. This is correct (no model = no tensors to register), but worth a comment to prevent future confusion.

---

10. ggml_backend_rpc_remove_server frees memory that may still be referenced

ggml-rpc.cpp — ggml_backend_rpc_remove_server calls delete ctx, delete reg, and delete dev for the registration context and devices. But if any ggml_backend_rpc_buffer_context objects still hold shared_ptr<socket_t> references obtained through this device, those references become dangling. The reg_map.erase(it) removes the lookup entry, but existing buffer objects still point to the freed memory.

Fix: Document that ggml_backend_rpc_remove_server must only be called after all buffers allocated through the peer are freed (i.e., after hydra_remove_combined_rpc_backend clears the bindings and the scheduler is rebuilt without the peer backend).

---

Summary

Severity	#	Issue
Critical	1	No-model path backend leak + ggml_backend_load_all before llama_backend_init
Critical	2	g_current_peer static not thread-safe
Critical	3	sched_reserve() called mid-lifecycle without decode-in-flight guard
Medium	4	const_cast on backend pointers
Medium	5	clear_combined_bindings clears all layers unconditionally
Medium	6	Forward declarations should be in shared header
Minor	7	Stripped flags silently consumed
Minor	8	Split mode hardcoded to "solo" — verify CombinedCapable chain
Minor	9	No-model path missing comment about no tensor registration
Minor	10	remove_server frees memory while buffers may still reference it

The three critical issues (#1, #2, #3) should be fixed before merge. The medium issues can be addressed in a follow-up if needed.

[ddvnguyen]

Review fixes pushed in commit 9c08000:

Addressed (all critical + medium)

Sev	#	Issue	Fix
Critical	1	Init order + backend leak	Moved llama_backend_init() before peer connection; added comments
Critical	2	g_current_peer thread-safety	Moved from static to server_context_impl::hydra_current_peer (task queue serialized)
Critical	3	sched_reserve() mid-lifecycle	Added CAUTION doc block + caller guarantee comment
Medium	4	const_cast on backends	Changed start_rpc_server to take vector by move; lambdas use mutable
Medium	5	clear_combined_bindings ALL layers	Added comment: single-peer assumption by design
Medium	6	Inline extern declarations	Added GGML_BACKEND_API declarations to ggml-rpc.h; removed inline externs
Minor	7	Stripped flags silence	Added LOG_WRN for --ggml-rpc-port, --peer-only, --combined-ot-pattern
Minor	9	No-model tensor registration	Added doc comment explaining no tensors without model

Items that are by design / out of scope

• [fork/hydra-346] qwen35moe model-load warmup hang — NOT the checkpoint gate (revert would regress hybrid KV restore) #8 (split-mode hardcoded to "solo"): The set_hydra_capabilities call is a placeholder — split mode comes from the request at runtime. CombinedCapable is set dynamically when rebind succeeds.
• fork: surface HYDRA_BUILD_TYPE in --version (hydra_vortex #349) #10 (remove_server frees referenced memory): Documented in ggml-rpc.h that caller must free all peer buffers first. This is satisfied by the hydra_remove_combined_rpc_backend call chain (clears bindings → rebuilds scheduler → removes RPC server).

[ddvnguyen]

Architecture-fit review (vs #29 plan + Hydra invariants)

Not re-treading the line-level review above (leaks, g_current_peer, const backends — raised and addressed in 9c08000). This pass is about fit with the #29 design and our multi-slot/COMBINE invariants, reading the diff at 9c08000.

Faithful to the plan ✅

• Protocol detection is exactly as specced: single listener, MSG_PEEK one byte, 0x0E→ggml-RPC, else→Hydra (0x30-0x46), byte not consumed.
• split_mode correctly dropped from the runtime path — the SET_EXPERT_MODE handler parses only {"mode","peer"}, so split geometry stays baked in at model load. That's the right call (see Unified RPC Server: one port, one binary, no extra flags #29 comment); split geometry can't be per-request without the deferred graph-routing work.
• Peer moved to the control plane (SET_EXPERT_MODE RPC op, Core-driven) rather than the HTTP inference body Unified RPC Server: one port, one binary, no extra flags #29 described. This is better — control/data separation, and it sidesteps the untrusted-body trust surface. But it diverges from the written Unified RPC Server: one port, one binary, no extra flags #29 plan, so Unified RPC Server: one port, one binary, no extra flags #29 should be updated so nobody later builds the HTTP-body version.

Gaps

1. (blocker) Runtime peer-switch rebuilds the scheduler with no in-flight guard — critical #3 is not actually fixed.
server-context.cpp:3568 calls hydra_remove_combined_rpc_backend() from the SET_EXPERT_MODE handler, which runs build_backend_buffer_vectors() + sched_reserve() (destroys/recreates the scheduler). The fix for critical #3 was doc-only — no if (is_processing()) return false; was added. The comment claims task-queue serialization makes it safe, but that only guarantees no task runs concurrently, not that slots are idle. If slot A is mid-generation (is_processing()==true) when a switch for slot B arrives, the rebuild invalidates the scheduler/bindings slot A depends on — this is the multi-slot vs global-single-peer teardown mismatch (also why clear_combined_bindings nulling all layers matters).
Fix: add the real if (is_processing()) return false; guard, or change the comment to state the actual requirement (Core must quiesce all slots before a peer switch) instead of claiming the queue already provides it.

2. Duplicated accept loop + dead null-this code.
The no-model path (llama-engine.cpp:~1053) inlines its own copy of the accept + MSG_PEEK + dispatch loop, while start_rpc_server grew this != nullptr / has_hydra guards for that same case — which the no-model path never exercises. Result: two copies of protocol detection to keep in sync, plus defensive null-this branches that are dead (and would be UB if reached — non-static member call through null).
Fix: extract the accept loop into one free/static function taking optional queue pointers; call it from both paths; delete the null-this branch.

3. Handler-level const_cast + two peer fields that can drift.
server-context.cpp:~3565: const_cast<server_context_impl *>(this)->hydra_peer = peer_override; permanently mutates config from a const handler, in addition to the new hydra_current_peer. Two members now track the active peer and can disagree. (The by-move fix addressed the other const_cast, not this one.)
Fix: make the handler non-const / route the mutation through impl, and keep one source of truth for the active peer.

Fit note

The peer-switch safety story rests entirely on Core's "only borrow a free peer GPU" exclusivity guarantee — a legitimate stance, but the engine has zero defense-in-depth, and #1's comment claims an engine-level guarantee it doesn't provide. Given llama-server is inherently multi-slot, the is_processing() guard would make a Core bug degrade to a rejected switch instead of corrupted slots.

Net: faithful to #29, and in one respect (control-plane peer) an improvement. One blocker (#1), two cleanups (#2, #3), two doc syncs (update #29's request-body example + note the peer-channel divergence).

---

Generated by Claude Code

[ddvnguyen]

Architecture review fixes pushed in commit 21a064d:

Addressed (all 3 gaps)

Gap	Issue	Fix
1 (blocker)	Peer switch mid-decode corrupts scheduler	Added is_processing() slot check — rejects switch with HYDRA_STATUS_BUSY if any slot is active
2	Duplicated accept loop + dead null-this	Extracted start_rpc_accept_loop() into server-rpc.h — both paths call the same function
3	const_cast + two peer fields drifting	Removed const_cast — hydra_peer is no longer mutated. hydra_current_peer is the only source of truth at runtime

[ddvnguyen]

Should-have (this PR): make the unified engine startup observable + health-checkable in PROD

While reviewing the unified server I looked at the startup path, because in PROD the engine takes 180–300s+ to load and it's currently very hard to tell "still loading" from "crashed." Since this PR already owns the llama-engine.cpp startup/start_rpc_server sequence and the no-model HTTP-health path, these belong here rather than a separate PR.

Why (the actual problem)

In the model path, the ordering today is:

load_model()                 // llama-engine.cpp:586 — the blocking 180–300s (incl. warmup)
start_rpc_server()           // ~637
ctx_http.init() + start()    // ~660 / ~860 — HTTP only begins listening HERE
is_ready.store(true)         // ~876

So /health is not listening until the model is fully loaded. For the entire 180–300s, hydra-head's probe gets connection-refused — indistinguishable from a crashed process. That's the whole reason health checks are hard to run in PROD here.

Two things make the fix low-risk:

1. /health is a public endpoint, already exempt from the is_ready middleware (server-http.cpp:31, :178-195) — it can answer before the model is ready, as long as the socket is listening.
2. ctx_http.start() is non-blocking — it spawns the listener on its own thread (server-http.cpp:385).
3. Upstream tools/server/server.cpp already does the correct order: ctx_http.start() (:291) → ctx_server.load_model() (:306) → ctx_http.is_ready.store(true) (:316). llama-engine.cpp is the odd one out. The no-model/peer-only path in this very PR (llama-engine.cpp:378) also already serves health first. We're just making the model path consistent.

---

A. Start HTTP + /health before load_model() (core fix)

Why: gives hydra-head a real signal during load, so liveness (process up, HTTP answering) is separable from readiness (model loaded + warmed). Set a short liveness timeout and a long (>300s) readiness timeout in the probe.

How — reorder the model path in llama_engine() to mirror server.cpp:

1. Move server_http_context ctx_http; ctx_http.init(params); and ctx_http.start() to before ctx_server.load_model(params).
2. Register /health, /version immediately; register inference routes too (they stay unreachable — the is_ready middleware 503s them until we flip the flag).
3. Keep load_model() on the main thread after start() (no background thread needed — the listener is already on its own thread). Flip routes.update_meta(ctx_server); ctx_http.is_ready.store(true); after load + capability setup, exactly as today at :876.
4. start_rpc_server() stays after load_model() (it needs the built backends) — unchanged.

Net: HTTP answers within ~milliseconds of process start; the model loads behind it.

B. Stage-aware /health payload

Why: so the probe (and a human debugging) sees where startup is, not just up/down.

How: replace the hardcoded {"status":"ok"} at llama-engine.cpp:664 with a small std::atomic<int> g_load_stage the startup sequence advances (INIT → LOADING_MODEL → SETTING_CAPS → RPC_START → WARMUP → READY). Handler returns:

• 200 {"status":"ok","stage":"ready"} once is_ready
• 503 {"status":"loading","stage":"loading_model"} (etc.) while loading

503-with-a-body still means "alive, be patient" to the orchestrator, vs connection-refused = "dead." Keep /health exempt from the middleware (already is).

C. Per-phase timing logs (do this first — it's the debugging unblock)

Why: the 180–300s is currently one opaque common_init_from_params call (server-context.cpp:960) covering model I/O + CUDA context init + warmup (default on, mparams.warmup=true at :848). We don't actually know where the time goes, which is half of "hard to debug."

How: wrap each phase with a steady-clock delta and LOG_INF("eng: <phase> took %.1fs") — around load_model(), capability setup, start_rpc_server(), and (if separable) warmup. Cheap, no behavior change, and it tells us whether the follow-ups below are worth it.

---

Follow-ups (NOT this PR — build/infra, gated on C's numbers)

• D. Persist the CUDA JIT cache across restarts. The container is ephemeral (reclaimed on inactivity), so ~/.nv/ComputeCache is cold every boot. If the sm86-sm120 fat binary ships PTX (not real cubin) for the running arch, first launch JIT-compiles the kernel set — minutes, and it fits the observed variance. Set CUDA_CACHE_PATH to a mounted volume and/or ensure real cubins for sm_86 and sm_120a. Likely the biggest raw-time cut — but confirm with C's logs first.
• E. Model on tmpfs + explicit warmup stage. Confirm the GGUF is mmap'd off /mnt/llm-ram (tmpfs), not a slow bind mount; make warmup a logged stage with --no-warmup available for dev/debug.

---

Acceptance criteria

• curl /health returns 503 {"stage":"loading_model"} within ~1s of process launch (not connection-refused)
• /health flips to 200 {"stage":"ready"} only after model load + warmup + capability setup
• Startup logs show per-phase durations
• hydra-head probe distinguishes loading (retry) from dead (restart) via HTTP status, with separate liveness/readiness timeouts

---

Generated by Claude Code

[ddvnguyen]

Re-scoping the startup should-haves out of this PR

On reflection the startup/health work above is a separate concern from the unified RPC server and shouldn't gate this PR. Moved it to a dedicated Phase E — Observable startup & staged health on the design issue: #29 (comment)

This PR stays scoped to Phase A (unified RPC server). The line-level handoff comment above remains valid as the implementation reference for Phase E.

---

Generated by Claude Code

[ddvnguyen]

Verified — all review findings resolved at 21a064d; merging

(Posting as a comment since GitHub won't let the author self-approve.)

Confirmed the three architecture findings are properly addressed in 21a064d:

• Merge upstream b9501 + fix: context-checkpoint reuse for recurrent/hybrid (qwen35moe) #1 (blocker) — unguarded scheduler rebuild on runtime peer-switch: ✅ SET_EXPERT_MODE now checks all slots and rejects the switch with HYDRA_STATUS_BUSY if any is is_processing(), instead of calling hydra_remove_combined_rpc_backend → sched_reserve() mid-decode. Latent multi-slot corruption → safe rejection.
• feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260) #2 — duplicated accept loop + dead null-this: ✅ Extracted to shared start_rpc_accept_loop (server-rpc.h), used by both paths; start_rpc_server keys off impl/queue pointers, UB gone; Windows stub included.
• Feat/m perf 9 engine model identity #3 — handler const_cast + dual peer field: ✅ Removed; hydra_current_peer is the single source of truth.

Lower-level criticals (backend leak, g_current_peer, const backends) were handled in 9c08000.

For the record: this fork has no CI (total_count: 0), so this review is the verification of record — no independent CUDA build was run; relying on the author's clean-compile report for sm86/sm120. Non-blocking nits: busy-reject log prints total slot count vs active; --tensor-split parser doesn't validate split-count vs device-count (testing-only flag).

Scope is Phase A (unified RPC server); startup/health work is split to Phase E on #29. Merging via squash.

---

Generated by Claude Code