[ddvnguyen]

Summary

Part of ggml-org#470 ("Merged P/D StatePut + Decode with GGUF-derived model identity"), Phase 2 of a 4-phase rollout. Depends on Phase 1 (#63, GGUF identity getters) — base branch is feat/470-gguf-model-identity, not hydra-fork directly; rebase target after Phase 1 merges.

Rewrites the binary DECODE (0x43) RPC. Today it sends the RPC response header before generation starts, then streams raw token frames — a real, already-hit wall documented in-code ("DECODE sends response header before task completes... cannot carry model identity in RPC response meta"). There were zero production callers of that shape (confirmed via Core-side audit), so this is a clean rewrite, not a back-compat-preserving change.

New design: merge STATE_PUT + decode-kickoff into one framed request that validates model identity, restores KV, and returns a decode_request_id immediately — generation runs async, retrieved via new GET/DELETE /v1/decode/{id} HTTP endpoints. This is the PREFILL → save slot → DECODE (get a request id) → HTTP streams result flow.

Architecture (two-phase, went through 3 review rounds to get right)

RPC thread                          Inference thread
────────────                        ────────────────
hydra_handle_decode()
  read framed [json_len][json][KV]
  post HYDRA_ENGINE_DECODE task  ──►  identity validation (GGUF getters vs kv_metadata)
  recv_with_timeout(30s)              KV restore (llama_state_seq_set_data) — only if valid
  ◄──────────────────────────────   send real match/valid/restore_slot_ms
  build REAL RPC response (not a
    hardcoded placeholder)
  post SERVER_TASK_TYPE_COMPLETION ►  launch via normal task path
  return decode_request_id              update_slots() drives generation
                                         (PROCESSING_PROMPT → DONE_PROMPT → GENERATING)
                                       background thread drains queue_results,
                                         writes final result + hydra_metrics into
                                         decode_results buffer
GET /v1/decode/{id}  ──────────────►  reads decode_results buffer (SSE or buffered JSON)

Two structural issues came up in review and are fixed in this PR (not left as known gaps):

1. Thread starvation: an earlier iteration ran the full generation loop as a blocking while inside process_single_task — the single thread shared by every opcode and every slot on the engine instance. That would have frozen the entire engine (no other PREFILL/STATE_GET/etc.) for the duration of any in-flight decode. Fixed by reusing the existing SERVER_TASK_TYPE_COMPLETION + update_slots() machinery — the same mechanism every other completion in this server already uses — instead of a hand-rolled loop.
2. Fake immediate response: an earlier iteration returned {"valid": true, ...} unconditionally from the RPC handler without waiting for actual validation. Fixed by having hydra_handle_decode synchronously wait (via recv_with_timeout, same pattern as hydra_handle_configure) for the validate+restore phase, so the immediate response reflects the real result — callers can trust valid/match without polling.
3. Orphaned result buffer (caught in final review, fixed directly rather than another round-trip): the fix for Merge upstream b9501 + fix: context-checkpoint reuse for recurrent/hybrid (qwen35moe) #1 replaced the generation loop with a posted SERVER_TASK_TYPE_COMPLETION task, but nothing consumed that task's result — GET /v1/decode/{id} would 404 on every request, even successful ones. Added a detached background thread (mirrors the existing STATE_GET background-thread pattern in this file) that drains queue_results for the completion task and populates decode_results.

Changes

• tools/server/server-context.cpp — hydra_handle_decode rewritten for the framed wire format + synchronous validate+restore wait; SERVER_TASK_TYPE_HYDRA_ENGINE_DECODE case does identity validation (via Phase 1's GGUF getters) + KV restore only, then posts a SERVER_TASK_TYPE_COMPLETION task; new background thread populates the result buffer; GET/DELETE /v1/decode/:decode_request_id handlers; merged_decode added to INFO (0x41) capabilities.
• tools/server/server-task.h/.cpp — hydra_action gains decode_json/kv_data/decode_request_id; server_task_result_hydra_engine gains decode_request_id/match_valid/match_json/restore_slot_ms/decode_init_ms/id_slot.
• tools/server/server-context.h — decode_result_entry struct + bounded buffer (HYDRA_DECODE_RESULT_TTL_S/HYDRA_DECODE_RESULT_MAX env vars, default 300s/1024 entries).
• tools/server/server-rpc.h — HYDRA_MAX_JSON_HEADER (32 KiB cap).
• tools/server/server-http.h/.cpp, server.cpp — del() HTTP method support, route registration.

Test plan

• cmake --build build --target llama llama-server-impl — clean compile, verified independently by me (reviewer) after every round of changes, not just agent self-reports.
• Not yet tested against a live engine instance / real multi-slot concurrent load — E2E verification (including confirming the thread-starvation fix under real concurrent traffic) is a later, separate step.

Known gaps (acceptable, documented in-code)

• t3_reloaded/t3_reload_ms in the hydra_metrics sidecar remain stubbed (false/0.0) — real T3-reload-state plumbing is out of scope for this PR.
• DELETE /v1/decode/{id} cancellation is best-effort: removes the buffered result but does not interrupt in-flight generation.
• stop type tracking has minor gaps in edge cases.

Notes for reviewer

Does not touch STATE_PUT (0x31) — it's unchanged, still used by engines that don't advertise merged_decode. Does not touch hydra_classify_config_key() or CONFIGURE (0x40) — out of scope, those are ggml-org#487's territory (already merged via PR #62).

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

[ddvnguyen]

QA Review — PR #64 (Phase 2: framed DECODE 0x43 rewrite)

Reviewed git diff feat/470-gguf-model-identity..feat/470-framed-decode-rpc in full context (server-context.cpp, server-context.h, server-task.{h,cpp}, server-rpc.h, server-http.{h,cpp}, server.cpp), plus server-queue.{h,cpp} for the server_response primitives the new code depends on. Verified the build independently: cmake --build build --target llama-server-impl compiles clean (only pre-existing unrelated deprecation/format warnings) after touching the three changed .cpp files to force recompilation.

Findings (ranked by severity)

1. (Blocking — race condition, silent result loss) decode_request_id is reused as the task id for two different posted tasks, racing on the shared waiting_task_ids set.
server-context.cpp:8784 sets val_task.id = decode_request_id; server-context.cpp:4215 later sets cmpl_task.id = decode_request_id — the same numeric id. server_response::send() (server-queue.cpp:319-332) only enqueues a result if its id is currently present in waiting_task_ids; otherwise it is silently dropped (no error, no log). Sequence:

• RPC thread: add_waiting_task_id(id) → posts val_task → recv_with_timeout gets the validation result → remove_waiting_task_id(id) (server-context.cpp:8789-8795).
• Inference thread, in the same task handler, right after sending the validation result, re-adds the same id for the completion task: queue_results.add_waiting_task_id(decode_request_id) (server-context.cpp:4224) then posts cmpl_task and spawns the background drain thread (server-context.cpp:4235-4242).

These two operations (RPC thread's remove_waiting_task_id cleanup vs. inference thread's add_waiting_task_id for the new task) are on different threads with no ordering guarantee between them. If the RPC thread's remove runs after the inference thread's add (a real possibility — the RPC thread must wake from a condvar wait and unwind a few stack frames, while the inference thread proceeds immediately), it erases the id the background thread just registered. When the completion later finishes and calls queue_results.send() with that id, waiting_task_ids no longer contains it → the result is dropped. The background thread's own recv_with_timeout(ids, 120) (server-context.cpp:4241) then times out, logs a warning, and returns — decode_results is never populated. GET /v1/decode/{id} then 404s forever for a request whose generation actually completed successfully. This is intermittent (timing-dependent), so it can pass casual/light testing and fail under concurrent load — exactly the "E2E verify not yet done" gap the PR itself flags. Fix: give the completion task its own distinct id (e.g. a second counter, or namespc the two waits so they don't collide on the same key in waiting_task_ids).

2. (Blocking — orphaned request / resource leak on RPC-side timeout) The 30s recv_with_timeout for validation+restore has no cancellation path, and the timeout response carries no decode_request_id.
server-context.cpp:8792-8801: if val_res_ptr is null (timeout), hydra_handle_decode writes HYDRA_STATUS_ERROR with zero-length meta (no id) and returns. The posted val_task is not removed from queue_tasks — it will still run eventually (single inference thread, tasks processed serially), performing the identity check, mutating slot state via llama_state_seq_set_data/prompt_clear, and then unconditionally posting the completion task and spawning the background consumer — entirely invisible to the RPC caller, who has no id to poll or cancel with. The slot stays "busy" (slot->is_processing(), gated at server-context.cpp:4066-4071) for the full KV-restore + generation duration, and the generated result sits in decode_results until TTL (default 300s) with no reachable key. Given CLAUDE.md's own numbers (~800MB KV blobs), and that this wait is for an inference-thread-serialized task (queued behind whatever else is in flight), 30s is not generous headroom under load — this is the scenario check #2 was specifically worried about, and the code has no mitigation (no task cancellation on timeout, no id surfaced for later recovery).

3. (Bug — messages path silently mistokenizes) Chat messages are dumped as raw JSON text instead of being run through the chat template.
server-context.cpp:4204-4212: if prompt.contains("messages"), the code does prompt_str = prompt["messages"].dump(); and feeds that raw JSON string straight into tokenize_input_prompts(...) — unlike the old atomic-prefill code this replaced, which correctly called oaicompat_chat_params_parse(...) to expand messages via the chat template before tokenizing (see the removed code, old diff lines ~56-77). If any caller of merged DECODE still supplies messages (the atomic/single-node fallback path this mirrors), the model will decode on literally-tokenized JSON syntax, not the intended chat-formatted prompt. If no caller ever sends messages here in practice (cross-GPU decode-continue always sends prompt/empty), this is currently dead/unreachable code, but it's wired up and will misbehave silently (no error) the moment something exercises it.

Verified as correct (matching the PR's claims / instructions' checklist)

• Identity-validation-before-restore ordering (top design check): confirmed. The !valid early-return/break (server-context.cpp:4137-4145 region) happens strictly before the "KV restore" section that calls llama_state_seq_set_data — invalid identity aborts without ever writing to the engine's live KV state. No corruption window found here.
• Async handoff claim: real. hydra_handle_decode returns to the RPC caller as soon as the validation task's result arrives (recv_with_timeout(val_ids, 30)); it does not block on the completion task. The generation genuinely runs via SERVER_TASK_TYPE_COMPLETION + update_slots(), not a hand-rolled loop — the thread-starvation fix is real.
• decode_results buffer locking: consistently guarded by decode_results_mutex on all three access paths (background writer, GET, DELETE). GET/DELETE correctly 404 on missing/expired keys with no UB observed.
• Slot double-booking: guarded — slot->is_processing() check (server-context.cpp:4066-4071) at the top of the DECODE task handler defers/rejects a second DECODE against a slot that's still generating, so DELETE-then-immediate-retry cannot corrupt a shared slot's KV mid-flight (matches the "one GPU = one compute task" invariant).

Minor / non-blocking

• DELETE /v1/decode/{id} only erases the map entry; it does not stop the in-flight background thread (documented as a known gap), but note the background thread will re-insert the entry via decode_results[decode_request_id] = ... (server-context.cpp:4288) once generation finishes even after a DELETE, since there's no tombstone/cancelled flag. Not corruption, but contradicts the intuitive "delete removes it" semantic — the entry can reappear until TTL.
• decode_init_ms (server-task.h) is declared and surfaced in to_json() but never assigned anywhere — always 0, dead field.
• capabilities_diff_bits/model_capabilities_match softness is intentional and out of scope per the review brief (Phase 1's known CODE-bit heuristic issue).

Verdict

Findings #1 and #2 are concurrency-correctness bugs in the core mechanism this PR exists to build (reliable async result delivery), not edge-of-scope nits — they directly undermine the "fixed, not left as a known gap" claims in the PR description. Recommend fixing before merge.

REQUEST_CHANGES

1. Give the completion task (cmpl_task) its own distinct task id, separate from decode_request_id/val_task.id, so the two waits never share a key in server_response::waiting_task_ids (server-context.cpp:8784, 4215).
2. On RPC-side validation timeout, either cancel the in-flight val_task (e.g. SERVER_TASK_TYPE_CANCEL against its id) or surface decode_request_id in the timeout error payload so the caller has a way to poll/cancel/reap the orphaned request later (server-context.cpp:8797-8801).
3. Fix the messages path to route through oaicompat_chat_params_parse (or explicitly reject/ignore messages in this endpoint if it's genuinely never expected) instead of tokenizing dump()'d JSON (server-context.cpp:4204-4212).