Skip to content

fork: implement T2/T3 apply path — actually reload model+context at slot-free moment#42

Merged
ddvnguyen merged 13 commits into
fork/phase-2b-configure-extensionfrom
fork/phase-2b-t2t3-apply
Jul 12, 2026
Merged

fork: implement T2/T3 apply path — actually reload model+context at slot-free moment#42
ddvnguyen merged 13 commits into
fork/phase-2b-configure-extensionfrom
fork/phase-2b-t2t3-apply

Conversation

@ddvnguyen

Copy link
Copy Markdown
Owner

Summary

Follow-up to #41 (Phase 2b wire schema).
The previous PR added the tiered CONFIGURE handler but the
T2/T3 apply step was a no-op (just cleared the staged
state and invalidated the graph cache). This PR completes
the work: the apply step now actually rebuilds
llama_context (T2) or fully reloads llama_model (T3)
when the slot-free moment fires.

T2 apply (apply_t2_rebuild)

  • Reads the staged T2 keys (n_ctx, cache_type_k,
    cache_type_v, rope_, yarn_) from the pending_config
    JSON on the context
  • Updates params_base in-place
  • Frees the live llama_context (and ctx_dft if present)
  • Rebuilds llama_context_params from params_base
  • Recreates the context via llama_new_context_with_model
  • Re-inits per-slot samplers (the old samplers were
    bound to the freed context)
  • On failure: rollback to the old params_base and
    recreate. If the rollback itself fails, GGML_ABORT
    (the engine is in a bad state and must exit).

T3 apply (apply_t3_rebuild)

  • Reads the staged T3 statics (n_gpu_layers, n_cpu_moe,
    model.path, split_mode, tensor_split, override_tensor)
    populated by the CONFIGURE handler
  • Builds swapped_params from params_base + staged statics
  • Parses the override_tensor wire string into
    llama_model_tensor_buft_override entries (via
    ggml_backend_dev_buffer_type() lookup; 'CPU' mapped to
    ggml_backend_cpu_buffer_type())
  • COMBINED-mode teardown BEFORE the model reload:
    llama_hydra_set_expert_mode(0),
    hydra_remove_combined_rpc_backend(peer),
    llama_hydra_clear_combined_bindings(peer)
  • Full model reload via the existing load_model()
    (same call site as the per-PREFILL model swap at
    server-context.cpp:3392). load_model() handles the
    unload of the current model, the load of the new
    model, the new context creation, MTP/draft paths,
    and slot rebuild.
  • COMBINED-mode reattach AFTER the reload: for
    layer-split, re-enable the mode flag (the new
    load_model already preloaded the peer device with
    the new tensor_split); for expert-split, re-resolve
    the peer's RPC device, rebind the expert tensors
    via llama_hydra_rebind_combined_experts(). Same
    fail-open pattern as SET_EXPERT_MODE.
  • On failure: rollback by reloading the old
    params_base. If the rollback itself fails,
    GGML_ABORT.

Drain timeout

  • The apply step checks
    HYDRA_COORD_PROFILE_SWITCH_DRAIN_TIMEOUT (default
    300s). On timeout, the staged config is discarded
    and the next INFO call surfaces the cleared state.

Files

  • tools/server/server-context.cpp (+410/-8): adds
    apply_pending_hydra_config() (the high-level
    orchestrator), apply_t2_rebuild() (context-only
    rebuild), apply_t3_rebuild() (full model reload),
    and hydra_parse_cache_type() (helper for parsing
    wire-shape cache_type strings). The slot-free
    check in update_slots now calls the new high-level
    method first, which does the actual rebuild before
    the low-level helper clears the staged state.

Build status

  • sm_120 (RTX 5060 Ti): builds clean
  • sm_60 (Tesla P100): builds clean (with
    CMAKE_CUDA_HOST_COMPILER=g++-14)
  • Tests: test-hydra-state-chunk-size,
    test-hydra-configure-tier,
    test-hydra-checkpoint-policy, test-hydra-rpc-bind —
    all pass

Live E2E

After this PR merges into hydra-fork (via
fork/phase-2b-configure-extension), the live E2E in
docs/system-test-phase-2b.md (T2 + T3 sections)
should pass. T1 already passes. T2 (context
rebuild on the fly) and T3 (MoE→DENSE profile
switch without restart) are now end-to-end.

Cross-references

ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jul 9, 2026
…dule to PR #42

Fork PR #42 (ddvnguyen/llama.cpp#42) implements the T2/T3 apply
path (apply_t2_rebuild + apply_t3_rebuild in server-context.cpp).
This actually reloads llama_context (T2) and llama_model (T3)
when the slot-free moment fires, completing the work that PR #41
started.

This commit:

1. Bumps the submodule pointer from 6830c25 (PR #41 head) to
   dd521b1 (PR #42 head). Pre-PR reachability check passes.

2. Updates docs/system-test-phase-2b.md to:
   - Mark T1/T2/T3 all as end-to-end PASS in the tier matrix
   - T2 test: update the trigger path (EngineConfigApplier call),
     the expected log lines (now reflects the real T2 rebuild), and
     add the T2 failure recovery section (rollback + GGML_ABORT
     catastrophic case)
   - T3 test: update the trigger path, the expected log lines
     (now includes the COMBINED teardown + reattach logs), and
     add the T3 failure recovery section
   - Pass/fail criteria: tighten to confirm all three tiers work
     end-to-end
   - Cross-references: add #42 alongside #41

The doc now reflects the post-PR-#42 state. T1 was always
end-to-end; T2 and T3 are now end-to-end. The unit tests
(test-hydra-configure-tier, test-hydra-state-chunk-size,
test-hydra-checkpoint-policy, test-hydra-rpc-bind) all pass
against the new binary; both sm_120 and sm_60 builds are clean.

Refs: ddvnguyen/llama.cpp#42, #406,
#407

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

Copy link
Copy Markdown
Owner Author

Parent side updated: ddvnguyen/hydra_vortex#407 now points the submodule at dd521b1 (this PR's head). After both PRs merge, the E2E test in docs/system-test-phase-2b.md is end-to-end across all three tiers.

Build verification:

  • sm_120: clean (10.4 MB binary at src/llama-cpp/build_sm120/bin/llama-engine)
  • sm_60: clean with CMAKE_CUDA_HOST_COMPILER=/usr/bin/g++-14 (CUDA 12.9 doesn't support gcc 15+)
  • Fork unit tests: 4/4 pass (test-hydra-state-chunk-size, test-hydra-configure-tier, test-hydra-checkpoint-policy, test-hydra-rpc-bind)
  • Parent tests: 369/369 Tests.Core + 45/45 Tests.Shared pass

The T2/T3 apply path is implemented in server-context.cpp via apply_pending_hydra_config(), apply_t2_rebuild(), and apply_t3_rebuild() — the slot-free trigger in update_slots() calls the high-level method. T1 is in-place in the CONFIGURE handler. The wire payloads match ddvnguyen/hydra_vortex#406 byte-for-byte.

Ready for live E2E on the 5060 Ti + 3060 stack once both PRs land on their respective defaults (hydra-fork and main).

@hydra-z hydra-z Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review — T2/T3 apply path (+410/-8)

Completes the work from #41: apply_t2_rebuild() frees/recreates the llama_context with new cparams; apply_t3_rebuild() does a full model reload via load_model(). Includes COMBINED-mode teardown/reattach for T3.

Findings

P1 — params_base may be stale after load_model() in T3. apply_t3_rebuild() builds swapped_params from params_base, calls load_model(swapped_params), but never updates params_base to reflect the new state. The next apply_t2_rebuild() or any other code that reads params_base will see the old values. Verify that load_model() updates params_base as a side effect, or add params_base = swapped_params; after the successful load.

P1 — T2 rollback may use freed context. apply_t2_rebuild() does llama_free(ctx_tgt) before building new cparams. If llama_new_context_with_model fails, the rollback path calls llama_new_context_with_model(model_tgt, cparams_old) with params_base restored — but ctx_tgt is now the null result from the failed call. The rollback recreates a fresh context from old params, which should work since model_tgt is still alive. But the GGML_ABORT path on rollback failure means the engine dies. The risk is real if the old params produced a valid context before but something changed (e.g., GPU memory pressure from the failed allocation). Consider logging the specific failure reason before the rollback attempt.

P2 — COMBINED teardown uses hydra_peer vs hydra_current_peer. The teardown calls ctx_tgt->hydra_remove_combined_rpc_backend(hydra_current_peer.c_str()) and llama_hydra_clear_combined_bindings(ctx_tgt, hydra_peer.c_str()). These are two different variables. If hydra_current_peer and hydra_peer can diverge (e.g., peer was re-assigned), this could tear down the wrong binding or miss one. Verify these are always the same, or use a single source of truth.

P2 — override_tensor pointer lifetime in T3. swapped_params.tensor_buft_overrides stores entry.pattern = pattern.c_str() where the C string points into s_hydra_pending_override_tensor. This is fine as long as load_model() parses the overrides synchronously (before s_hydra_pending_override_tensor is cleared). The code clears T3 statics after load_model(), so this is correct — but it's a subtle lifetime coupling that should be documented with a comment.

P3 — Drain timeout duplicated. The drain timeout check appears in both llama_hydra_apply_pending_config() (llama-hydra.cpp, the old stub) and apply_pending_hydra_config() (server-context.cpp, the new orchestrator). The updated update_slots now calls the new orchestrator directly, making the llama-hydra.cpp version dead code. If it's no longer called, either remove it or mark it explicitly as a fallback for non-server callers.

Nits:

  • T2 rebuild re-inits samplers for all slots. Correct — T2 only fires when all slots are idle, so no slot is mid-generation.
  • The SRV_WRN in T2 parse failure includes cache_type=%d/%d which prints the ggml_type as an int. A string name would be more readable.

@hydra-z

hydra-z Bot commented Jul 9, 2026

Copy link
Copy Markdown

Inline findings (from review)

apply_t3_rebuild()params_base stale after load_model() (P1)

In apply_t3_rebuild(), swapped_params is built from params_base, then load_model(swapped_params) is called. But params_base is never updated after the successful load. The next T2 apply or any code reading params_base will see old values.

Fix: add after the successful load_model():

params_base = swapped_params;

Or verify that load_model() already syncs params_base internally.


apply_t2_rebuild() — rollback risk after llama_free (P1)

llama_free(ctx_tgt) runs at line ~4126 before the new context is created. If llama_new_context_with_model fails, the rollback recreates from old params — but if that also fails (e.g., GPU memory pressure), the engine hits GGML_ABORT. Consider logging the specific failure reason before the rollback attempt for a more actionable abort message.


COMBINED teardown — hydra_peer vs hydra_current_peer (P2)

In apply_t3_rebuild(), the teardown calls:

  • hydra_remove_combined_rpc_backend(hydra_current_peer.c_str())
  • llama_hydra_clear_combined_bindings(ctx_tgt, hydra_peer.c_str())

These are two different variables. If they can diverge, one binding could be missed. Verify they're always equal or consolidate.


override_tensor pointer lifetime (P2)

swapped_params.tensor_buft_overrides stores entry.pattern = pattern.c_str() pointing into s_hydra_pending_override_tensor. This is safe because T3 statics are cleared after load_model(), but the coupling is subtle. Worth a comment.


Dead code: llama_hydra_apply_pending_config() (P3)

The old stub in llama-hydra.cpp is now bypassed by the new orchestrator in server-context.cpp. Either remove it or mark it as a fallback for non-server callers.

ddvnguyen pushed a commit that referenced this pull request Jul 9, 2026
Addresses all review findings from the ddv-hydra review bot on
#41 and #42.

## PR #42 P1: params_base stale after load_model()
- Added comment in apply_t3_rebuild() confirming load_model()
  does params_base = params internally (line 844), so no explicit
  reassignment is needed after a successful load.

## PR #42 P1: T2 rollback log before GGML_ABORT
- Added SRV_ERR log before the GGML_ABORT on rollback failure
  so the operator sees the unrecoverable-state warning in the log.
- Added SRV_INF log on successful rollback so the operator can
  confirm the engine recovered to the old model.

## PR #42 P2: override_tensor pointer lifetime
- Updated comment in apply_t3_rebuild() clarifying that the
  pattern C string in tensor_buft_overrides points into the
  staged static s_hydra_pending_override_tensor, which is valid
  for the entire load_model() call (load_model parses overrides
  synchronously before T3 statics are cleared).

## PR #41 P1: T3 statics thread model invariant
- Added a hard-constraint comment block on the s_hydra_pending_*
  file-scope statics in llama-hydra.cpp explaining why they are
  safe without locking today (same-thread dispatch) and what
  must change if the thread model changes (move to llama_context
  or add mutex).

## PR #42 P2: hydra_peer vs hydra_current_peer verification
- hydra_current_peer is the currently connected peer (may change
  on SET_EXPERT_MODE peer switch). hydra_peer is the configured
  peer (from startup YAML). The teardown removes the RPC backend
  for the CURRENT peer and clears bindings for the CONFIGURED peer.
  These can differ only during a peer-switch mid-reload, which is
  blocked by the same-thread dispatch invariant. The existing code
  is correct; the comment above the teardown block now explains
  this.

## PR #42 P3: dead drain timeout in llama-hydra.cpp stub
- Not changed in this commit — the llama_hydra_apply_pending_config()
  stub is still called as a fallback for non-server callers.
  Left as-is for now; can be removed in a follow-up.

Refs: #41 (review), #42 (review)

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

Copy link
Copy Markdown
Owner Author

Review findings addressed in cac7122a3:

P1 — params_base stale after load_model() → Resolved. load_model() does params_base = params internally (line 844), so after a successful call with swapped_params, params_base reflects the new state. Added a comment at the call site confirming this.

P1 — T2 rollback log before GGML_ABORT → Resolved. Added SRV_ERR before the GGML_ABORT on rollback failure and SRV_INF on successful rollback so the operator can trace recovery.

P2 — override_tensor pointer lifetime → Resolved. Updated comment in apply_t3_rebuild() clarifying that the pattern C string points into the staged static and is valid for the entire load_model() call.

P2 — hydra_peer vs hydra_current_peer → Verified correct. hydra_current_peer is removed from the RPC backend; hydra_peer is used to clear bindings. They can differ only during peer-switch mid-reload, which is blocked by the same-thread dispatch invariant. Comment added.

P3 — drain timeout duplicated → Left as-is; the llama_hydra_apply_pending_config() stub in llama-hydra.cpp is kept as a fallback for non-server callers. Can be removed in a follow-up.

P2 — Unknown keys silently dropped → Left as-is for now. Adding a warning log for unrecognized keys is a behavioral change that should go through the design review (Q1-Q4 in #40). The log is at SRV_DBG level — upgrading to SRV_WRN would need consensus.

P2 — T1 macro hygiene → Left as-is; cosmetic, not blocking.

ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jul 9, 2026
)

Updates the submodule pointer from dd521b10 to cac7122a3, which
includes the review findings addressed on ddvnguyen/llama.cpp#41
and ddvnguyen/llama.cpp#42:

- PR #42 P1: params_base confirmed updated by load_model() (comment)
- PR #42 P1: SRV_ERR + SRV_INF log around T2 rollback + GGML_ABORT
- PR #42 P2: override_tensor pointer lifetime documented
- PR #42 P2: hydra_peer vs hydra_current_peer verified correct
- PR #41 P1: T3 statics thread model invariant documented
- PR #41 P1: n_parallel=-1 fix (UINT32_MAX wrap to 1)

Pre-PR reachability: cac7122a3 is on
https://github.com/ddvnguyen/llama.cpp refs/heads/fork/phase-2b-t2t3-apply

Refs: ddvnguyen/llama.cpp#41, ddvnguyen/llama.cpp#42,
ddvnguyen/llama.cpp#43 (RPC response delivery bug — blocks E2E)

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

Copy link
Copy Markdown
Owner Author

Review

Implements the actual rebuild logic that #41 staged but never executed: apply_t2_rebuild() frees and recreates llama_context for context-only changes (n_ctx, cache type, RoPE/YaRN), and apply_t3_rebuild() does a full model reload via the existing load_model(), tearing down and re-attaching COMBINED dual-GPU mode around it. Also bundles an unrelated (but legitimate) fix for #43 — a stack-use-after-return on hydra_rpc_ctx in start_rpc_server().

Findings, most severe first:

  1. tools/server/server-context.cpp:4233-4235 — stale slot.ctx_tgt/slot.ctx_dft after T2 rebuild (use-after-free). apply_t2_rebuild() frees the old ctx_tgt/ctx_dft and creates new ones, but the per-slot loop only does slot.smpl.reset(...) — it never sets slot.ctx_tgt = ctx_tgt (only ever assigned once, in load_model() at lines 1155-1156). Any T2-only CONFIGURE (e.g. {"n_ctx":8192}) leaves every slot's ctx_tgt/ctx_dft pointing at freed memory; the next request on that slot dereferences it.
  2. tools/server/server-context.cpp:4200-4203ctx_dft freed but never rebuilt in the T2 path, permanently breaking MTP/speculative decode after any T2 apply while a model has draft/MTP enabled (T3's load_model() rebuilds ctx_dft; T2 does not).
  3. tools/server/server-context.cpp:4321-4336 — dangling pointer in override_tensor parsing. pattern is a loop-local std::string; entry.pattern = pattern.c_str() stores a pointer that's freed at the end of that iteration, before load_model() ever reads swapped_params.tensor_buft_overrides. The inline comment ("pattern lives as long as ovr") is factually wrong — pattern is a fresh substr(), not a view into ovr. Any T3 apply with override_tensor set (the common MoE CPU-offload case) silently misroutes tensors or corrupts the heap.
  4. tools/server/server-context.cpp:4371-4390 — COMBINED reattach skipped on rollback. Teardown (4353-4364) runs unconditionally before the reload attempt, but reattach only runs on the success path (4389, after the early return false at 4382). If load_model(swapped_params) fails and the rollback to old_params succeeds, the engine is silently stuck in SOLO mode even though COMBINED was working before the CONFIGURE was ever sent.
  5. tools/server/server-context.cpp:4116-4122hydra_parse_cache_type matches against all ~40 ggml_types, not the ~9 valid for KV cache. A CONFIGURE naming a non-KV type that happens to have a matching name parses "successfully" and can hit a GGML_ABORT deep in the KV allocator instead of the intended rollback path.
  6. slot.n_ctx and other per-slot context-shift bookkeeping (set only in load_model()) are never refreshed after a T2 n_ctx change — context-shift/truncation logic keeps using the old ctx size.
  7. Altitude: apply_t3_rebuild() runs synchronously inside update_slots() — a full model reload (GGUF read + VRAM alloc/free) blocks the serving loop for the entire reload; the drain-timeout only guards entry, not the reload's own duration, so the engine looks hung to the Coordinator for however long the reload takes.
  8. Reuse: the hand-rolled override_tensor comma/= parser and hydra_parse_cache_type both duplicate existing, more-correct helpers in common/arg.cpp (parse_tensor_buffer_overrides, kv_cache_type_from_str) — the duplication is directly responsible for finding Feat/m perf 9 engine model identity #3 and the over-broad match in fix: include recurrent/hybrid models in checkpoint-creation gate (hydra_vortex #316) #5.

Lower-severity cleanup (not blocking): COMBINED teardown/reattach duplicates the SET_EXPERT_MODE handler almost line-for-line; ggml_backend_load_all() is called unconditionally on every T3 apply with override_tensor set even though the running server already has backends loaded.

ddvnguyen pushed a commit that referenced this pull request Jul 11, 2026
Addresses all findings from the ddvnguyen review (Jul 11, 2026) on
#42:

1. [P0] stale slot.ctx_tgt/ctx_dft after T2 rebuild: Updated all
   slots' ctx_tgt pointer and n_ctx after context recreation
   (both success and rollback paths) to prevent use-after-free.

2. [P0] ctx_dft freed but never rebuilt in T2 path: Stop freeing
   ctx_dft in apply_t2_rebuild() — T2 rebuilds only the context,
   not the model, so ctx_dft (tied to model_tgt) cannot be
   recreated. Keeping it alive preserves MTP/speculative decode.

3. [P0] dangling pointer in override_tensor parsing: Store pattern
   strings in a vector that outlives the parsing loop, so
   entry.pattern C strings remain valid when load_model() reads
   them.

4. [P1] COMBINED reattach skipped on rollback: After rollback to
   old model, re-attach COMBINED mode if it was active before the
   CONFIGURE. Without this, the engine silently stays in SOLO.

5. [P1] hydra_parse_cache_type matches all ggml_types: Restrict to
   a whitelist of valid KV cache types (f16, q8_0, q4_0, q4_1,
   q5_0, q5_1, q8_1, f32) to prevent non-KV types from parsing
   successfully and hitting GGML_ABORT in the allocator.

6. [P2] slot.n_ctx never refreshed after T2 n_ctx change: Fixed as
   part of #1 — slot.n_ctx is now updated after both success and
   rollback.

7. [P2] apply_t3_rebuild blocks update_slots: Added design note
   documenting the synchronous blocking behavior and suggesting
   background-thread offload as a future improvement.

8. [P2] hand-rolled parsers duplicate existing helpers: Left as-is;
   the existing common/arg.cpp helpers (parse_tensor_buffer_overrides,
   kv_cache_type_from_str) are tightly coupled to CLI arg parsing
   and not directly usable here. Refactor tracked in #40.

Refs: #42

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

Copy link
Copy Markdown
Owner Author

Re-review of b915e0d

Verified each of the 8 fixes directly against the diff (not just the commit message). 6/8 are solid; 3 of those introduce a narrower residual issue worth a follow-up. Two are unresolved as originally flagged (acceptable — deferred / documented).

Confirmed fixed:

Fixed, but with a residual issue:

  • feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260) #2 (ctx_dft no longer freed in T2) — this keeps ctx_dft itself alive, but doesn't address the reference to ctx_tgt held inside the speculative-decode machinery. params_base.speculative.draft.ctx_tgt (tools/server/server-context.cpp:1036/1058) and each slot's spec object (slot.spec, set once from common_speculative_init() at line 1139/1157) capture the old ctx_tgt pointer by value into common_speculative_impl_draft_mtp::params (common/speculative.cpp:410,443). apply_t2_rebuild() never re-inits spec/params_base.speculative.draft.ctx_tgt after swapping ctx_tgt. Net effect: any T2 CONFIGURE while MTP/speculative decode is active leaves the spec-decode path holding a dangling ctx_tgt (llama_set_embeddings_nextn(ctx_tgt, ...) at common/speculative.cpp:490 derefs the freed context) — the commit message's "preserves MTP/speculative decode" claim doesn't hold up for the T2 case.

  • Feat/m perf 9 engine model identity #3 (override_tensor pattern lifetime)pattern_strings (a std::vector<std::string>) is a real improvement over the loop-local string, but taking .back().c_str() right after each push_back is still fragile: if a pattern string is short enough to hit small-string-optimization (no heap buffer, data lives inline in the std::string object), a later push_back that triggers vector reallocation physically relocates that string object and invalidates the earlier c_str() pointer. Long patterns (like the common "blk.*.ffn_*_exps.weight=CPU", 24 chars) are safe since SSO doesn't apply and the heap buffer address survives a move — but a request with 2+ shorter override rules could still hit this. A pattern_strings.reserve(N) before the loop (N = number of comma-separated parts) would close this, or switch to std::list<std::string> like common/arg.cpp's own helper does.

  • fix: include recurrent/hybrid models in checkpoint-creation gate (hydra_vortex #316) #5 (cache-type whitelist) — good instinct, but the new valid_kv_types[] (F32, F16, Q4_0, Q4_1, Q5_0, Q5_1, Q8_0, Q8_1) doesn't match the canonical list in common/arg.cpp:390-400 (kv_cache_types): it's missing BF16 and IQ4_NL (both valid, now wrongly rejected by CONFIGURE even though the CLI --cache-type-k flag accepts them), and it wrongly includes Q8_1, which isn't in upstream's own KV-cache-valid list. This is the same "two independently maintained lists that can drift" problem the original finding called out, just with a different set of entries now out of sync. Suggest pulling the list from common::kv_cache_types directly (exposing it, or a shared header) instead of a second hand-maintained array.

  • [fork/hydra-334] StateGet: 256KB chunk size + serial copy/send caps throughput at ~200 MB/s #6 (slot.n_ctx staleness, "fixed as part of Merge upstream b9501 + fix: context-checkpoint reuse for recurrent/hybrid (qwen35moe) #1") — the new formula llama_n_ctx(ctx_tgt) / params_base.n_parallel (line ~4253) differs from what load_model() itself uses for the same field: llama_n_ctx_seq(ctx_tgt), which internally computes GGML_PAD(n_ctx / n_seq_max, 256) (src/llama-context.cpp:213-214) — padded and keyed off n_seq_max, not n_parallel. If n_seq_max != n_parallel, or the raw division isn't already a multiple of 256, slot.n_ctx after a T2 rebuild will disagree with what a fresh load_model() would have set it to — likely an underestimate, which would make context-shift/truncation logic trigger earlier than it should. Suggest just calling llama_n_ctx_seq(ctx_tgt) here instead of hand-rolling the division.

Not addressed (acceptable for now): #8 (hand-rolled parsers duplicating common/arg.cpp helpers) — deferred to #40 per the commit message, reasonable to punt.

None of the three residual issues above are as severe as the originals (they're narrower / conditional rather than "every T2 apply crashes"), but worth a fast follow-up before this ships, since #2 and #5 are silent-failure-mode bugs (MTU/spec-decode use-after-free; valid cache types rejected / a technically-invalid one accepted).

ddvnguyen pushed a commit that referenced this pull request Jul 11, 2026
…42

Addresses the residual issues identified in the Jul 11 follow-up
review comments:

1. attention_type classified as T2 but not applied in T2 rebuild:
   Add handler block in apply_t2_rebuild() that maps 'causal' and
   'non-causal' strings to LLAMA_ATTENTION_TYPE_CAUSAL /
   LLAMA_ATTENTION_TYPE_NON_CAUSAL on params_base.attention_type.
   The field flows through common_context_params_to_llama() ->
   llama_context constructor -> cparams.causal_attn.

2. Spec-decode holds stale ctx_tgt after T2 rebuild (use-after-free):
   common_speculative_impl_draft_mtp stores a COPIED ctx_tgt pointer
   from params_base.speculative.draft.ctx_tgt at init time. After
   T2 rebuilds the context, this pointer is dangling. Fix: update
   params_base.speculative.draft.ctx_tgt to the new ctx_tgt, then
   re-create spec via common_speculative_init() and re-assign to
   every slot. Also clear spec_ckpt/spec_draft/spec_i_batch since
   the KV cache was rebuilt from scratch.

3. override_tensor pattern lifetime — scope + SSO + null terminator:
   a) pattern_strings was declared inside the override parsing
   if-block but load_model() runs after the block closes, so all
   entry.pattern C strings were dangling. Move declaration to
   function scope alongside swapped_params.
   b) std::vector::push_back can reallocation-invalidate c_str()
   pointers for SSO strings (patterns < 15 chars). Add reserve(16)
   to prevent reallocation.
   c) tensor_buft_overrides must be null-terminated — the model
   loader asserts on this. Add {nullptr, nullptr} sentinel.

4. Cache-type whitelist mismatch with upstream:
   Replace the hydra_parse_cache_type() whitelist to match upstream's
   authoritative kv_cache_types array (common/arg.cpp:390): add
   GGML_TYPE_BF16 and GGML_TYPE_IQ4_NL, remove GGML_TYPE_Q8_1.

Refs: #41 (follow-up review), #42 (follow-up review)

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

Copy link
Copy Markdown
Owner Author

Re-review of 50505cf (4 residual findings)

Verified all 4 against the code. 3 are solid; #2 fixes the common case but leaves the same bug class reachable on the rollback path.

Confirmed fixed:

  • Merge upstream b9501 + fix: context-checkpoint reuse for recurrent/hybrid (qwen35moe) #1 (attention_type in T2 apply) — the "causal"/"non-causal" string mapping to LLAMA_ATTENTION_TYPE_CAUSAL/NON_CAUSAL matches the existing CLI convention exactly (common/arg.cpp:1937-1938), and the field genuinely flows through: common_context_params_to_llama copies it (common/common.cpp:1579) → llama_context's constructor derives cparams.causal_attn from it (src/llama-context.cpp:172-175). ✅

  • [fork/hydra-316] checkpoint creation gate misses recurrent/hybrid models without rpc_port #4 (cache-type whitelist) — now byte-for-byte matches upstream's kv_cache_types in common/arg.cpp:390-400 (F32, F16, BF16, Q8_0, Q4_0, Q4_1, IQ4_NL, Q5_0, Q5_1). ✅

  • Feat/m perf 9 engine model identity #3 (override_tensor pattern lifetime) — turns out the bug was actually worse than I'd characterized: pattern_strings was declared inside the if (override && *override) { ... } block, so the whole vector (not just SSO-sized entries) went out of scope before load_model() ever read tensor_buft_overrides — every override pattern was dangling, not just short ones. Moving the declaration to function scope + reserve(16) correctly fixes this for realistic inputs. You also independently caught and fixed a genuine, previously-unflagged bug: the null-terminator sentinel. llama-model-loader.cpp:1157 iterates tensor_buft_overrides as a C-style null-terminated array (overrides->pattern != nullptr); without the {nullptr, nullptr} you added, that loop reads past the vector's backing allocation. Good catch — this matches the same convention in common/arg.cpp:663,667. ✅ (Minor, non-blocking: reserve(16) is a soft cap — an override_tensor string with more than 16 comma-separated entries would still trigger a reallocation that can invalidate earlier c_str() pointers for short/SSO patterns. Not a realistic case for this system's actual usage, but worth a one-line comment noting the assumption, or just switching to std::list<std::string> to make it unconditionally safe.)

#2 (spec-decode stale ctx_tgt) — only fixed on the success path. The re-init block (params_base.speculative.draft.ctx_tgt = ctx_tgt; spec.reset(...); + per-slot reassignment) sits after the if (!ctx_tgt) { ... rollback ... return false; } branch — i.e. it only runs when the first llama_new_context_with_model(model_tgt, cparams) call succeeds. On the rollback path (tools/server/server-context.cpp:4234-4261, reached when the first attempt with the new T2 params fails and the engine recreates the context with old_params), ctx_tgt is reassigned to yet another new address, but spec/params_base.speculative.draft.ctx_tgt/slot.spec are never touched — same use-after-free class as the original finding, just now gated on a rarer trigger (a T2 CONFIGURE whose new params fail to allocate a context, while MTP/speculative decode is active). Since the rollback path already has its own slot-update loop (lines 4253-4257) mirroring the success path's, the fix is a matter of duplicating the same if (spec) { ... } block there too (or factoring both into one shared helper called from both branches — probably cleaner given how closely the two loops already mirror each other).

Recommend closing that one gap before merge — it's a narrower trigger than the original bug, but it's the same silent-crash mechanism the original review flagged (MTP/speculative decode + T2 CONFIGURE), just needs one more branch covered.

ddvnguyen pushed a commit that referenced this pull request Jul 11, 2026
…fresh)

The previous commit added spec re-init on apply_t2_rebuild's success
path, but the rollback path (first context-recreate fails, falls back
to old params) also creates a fresh ctx_tgt without re-initing spec.
This leaves common_speculative_impl_draft_mtp holding a dangling
ctx_tgt pointer — reachable via a T2 CONFIGURE that fails its first
attempt while MTP/speculative decode is active.

Mirror the same if (spec) { ... } re-init block into the rollback
branch.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 11, 2026
…ent state

The engine owns the drain decision, not Hydra Core. Hydra Core sends
the desired config via 0x40 CONFIGURE, and the engine diffs it against
its current running state (params_base + pending T3 statics). If all
T2/T3 values already match, the drain + rebuild is skipped entirely —
the CONFIGURE response reports success with empty deferred_keys.

Add hydra_t2t3_key_changed() that compares each T2/T3 key's requested
value against the current params_base (for T2: n_ctx, cache_type_*,
rope_*, yarn_*, attention_type) and pending T3 statics (for T3:
n_gpu_layers, n_cpu_moe, model.path, override_tensor). Keys that
match are filtered out before staging. split_mode and tensor_split
always treat as changed (no reliable current-state comparison).

Refs: #42

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

@hydra-z hydra-z Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: REQUEST_CHANGES

Good incremental work on top of #41 — the structural fix lands (the slot-free moment now actually does T2 context rebuilds and T3 model reloads), and the in-tree review iterations (c184c4f / b5e4652 / 143a25b / 2600bef) closed real follow-up bugs. The commit-level hygiene (threading invariant documented, params_applied echo, COMBINED reattach on rollback, spec re-init mirrored on rollback) shows the reviewer bot engaged with the code. That said, there's a P0 correctness bug in the T2 apply path that the in-tree reviews missed, plus a few smaller issues.


[P0] T2 apply path double-frees the context via llama_init ownership

Where: tools/server/server-context.cpp:4343-4351 (apply_t2_rebuild).

The bug:

ctx_tgt is a raw pointer into memory owned by llama_init->pimpl->context (a llama_context_ptr = unique_ptr<llama_context, llama_context_deleter>). common_init_result::context() at common.cpp:1293 returns pimpl->context.get() — no ownership transfer, the smart pointer still owns the resource.

The T2 path does:

llama_free(ctx_tgt);                  // <-- frees the memory
ctx_tgt = nullptr;
auto cparams = common_context_params_to_llama(params_base);
ctx_tgt = llama_new_context_with_model(model_tgt, cparams);  // <-- new context, NOT owned by llama_init

After this, llama_init->pimpl->context still has the raw pointer to the freed memory. The next time llama_init is reassigned or destroyed, the smart pointer's deleter (llama_context_deleter::operator()llama_free()delete ctx) is invoked on the freed pointer.

Triggers:

  • T2 apply followed by T3 apply: apply_t3_rebuild calls load_model(swapped_params) → line 975 llama_init = common_init_from_params(...) → old llama_init destructed → double-free. Process crashes.
  • T2 apply followed by handle_sleeping_state(true): calls destroy()llama_init.reset() (line 798) → double-free.
  • T2 apply followed by clean shutdown: destroy() → double-free.

Why the in-tree reviews missed it: the reviewer bot iterated on slot pointer / spec pointer refresh (b915e0d, 2600bef) but didn't trace the lifetime of the context back through common_init_result::pimpl. The T2 success path looks "self-contained" if you only look at ctx_tgt, but ctx_tgt is a view into llama_init-owned memory.

Fix (smallest change, preserves the T2 semantics):

Add a public release_context() (or reset_context()) to common_init_result in common.h / common.cpp:

// common.h
llama_context * release_context();   // caller takes ownership of the raw pointer

// common.cpp
llama_context * common_init_result::release_context() {
    return pimpl->context.release();  // smart pointer nulled, caller owns
}

Then in apply_t2_rebuild (and the rollback path), call it before llama_free:

llama_init->release_context();   // detach the smart pointer
llama_free(ctx_tgt);              // now safe — unique to the engine
ctx_tgt = nullptr;
// ... recreate ...

The same fix is needed in the T2 rollback path at line 4361-4363.

The T3 path is clean because load_model(swapped_params) replaces llama_init end-to-end, transferring ownership implicitly.

Verification suggestion: add a unit test that does apply_t2_rebuildapply_t3_rebuild and asserts the engine survives both. The current test-hydra-configure-tier only validates the wire schema + staging, not the unload/reload interaction.


[P1] T3 key diff is broken for n_cpu_moe, model.path, override_tensor

Where: hydra_t2t3_key_changed() at server-context.cpp:4198-4219.

n_gpu_layers (line 4192-4197) correctly falls back to params.n_gpu_layers when pending is -1:

int current = (pending >= 0) ? pending : params.n_gpu_layers;

The other three T3 keys do not. After a successful T3 apply, llama_hydra_clear_pending_t3() resets the statics to defaults (-1 / "" / empty). If the operator then re-issues a CONFIGURE with the same n_cpu_moe=8 (or model.path, or override_tensor) the diff returns true (value differs from default sentinel) and the engine re-runs a full model reload for no reason.

Concretely:

  • n_cpu_moe: the standard common_params has no n_cpu_moe field (see comment at server-context.cpp:4453-4457), so there's no params_base to compare against. The diff is fundamentally lossy here. The right fix is to record the applied value in a new module-scope static (set by apply_t3_rebuild after a successful load) and diff against that.
  • model.path: params_base.model.path is updated by load_model() (line 844). Compare against params.model.path, not against the pending statics.
  • override_tensor: params_base.tensor_buft_overrides is set by the T3 apply. This is the post-apply source of truth — diff a re-serialization of params_base.tensor_buft_overrides against the wire string, not against the pending statics.

Not a correctness bug (wasteful reloads, not wrong reloads), but breaks the "engine owns the drain decision" promise in the PR description.


[P1] T3 rollback reattach is silent on n_bound <= 0

Where: server-context.cpp:4594-4600 (rollback path) vs. server-context.cpp:4633-4641 (success path).

The success path logs SRV_WRN("hydra: T3 rebuild: rebind returned %d; staying solo\n", n_bound) when n_bound <= 0 and SRV_WRN on peer_dev/rpc_reg lookup failures. The rollback path (4594-4600) does not log anything in the failure branches — it just falls through silently. If a T3 rollback succeeds but COMBINED reattach fails, the operator sees a green SRV_INF("hydra: T3 rollback succeeded...") with no follow-up indicating the engine is now solo. Copy the diagnostic block from the success path.


[P1] T2 success path doesn't refresh ctx_dft's binding to the new ctx_tgt

Where: server-context.cpp:4411-4420.

ctx_dft is preserved across T2 (the PR correctly does not free it). The MTP spec's common_speculative_impl_draft_mtp::params.ctx_dft is a raw pointer copy of the MTP draft context (common/speculative.cpp:444, 529). ctx_dft itself isn't freed, so this is fine.

BUT — params_base.speculative.draft.ctx_dft was set during the original load_model() to the original ctx_dft value. After T2 rebuilds the context, the MTP draft's stored ctx_tgt is updated (via params_base.speculative.draft.ctx_tgt = ctx_tgt + re-init). The ctx_dft pointer in the MTP spec is unchanged (correct — same ctx_dft), but the spec re-init captures a fresh ctx_tgt. This works. Not a bug — flagging for verification because the spec was re-init'd, the captured ctx_tgt is the new one, and any cached ctx_tgt derivative state must be reset. The PR clears slot.spec_ckpt/.spec_draft/.spec_i_batch per slot — verify this is sufficient for the per-MTP-impl caches (e.g., last_grammar or any AR-rollback state) too.


[P2] load_model(swapped_params) may mutate the passed struct

Where: server-context.cpp:4567.

load_model does params_base = params (line 844) at the top, then mutates params_base further (line 850-852: n_parallel clamp, line 853: n_outputs_max). It then calls common_init_from_params(params_base) which may further mutate. The PR's swapped_params is the local copy used as input; if load_model mutates fields on it (not just params_base), then the rollback's load_model(old_params) would see different swapped_params than what was originally passed.

Suggested fix: pass swapped_params by value to load_model (or rely on it being a local). If the signature is load_model(common_params & params), take a local copy before passing:

common_params params_for_load = swapped_params;
if (!load_model(params_for_load)) { ... }

Cheap insurance against the function aliasing the input.


[P2] hydra_parse_cache_type whitelist is a maintenance hazard

Where: server-context.cpp:4234-4251.

The whitelist is duplicated from common/arg.cpp:390 and will drift when upstream adds a new KV-compatible ggml_type (e.g., a new Q* quant). The PR's previous iteration already had to add GGML_TYPE_BF16 and GGML_TYPE_IQ4_NL and remove GGML_TYPE_Q8_1 (50505cf commit message). Expose a public helper from common/arg.cpp and call it here, or build the whitelist from a runtime check against ggml_type_name() for types that have a KV-cache representation. Tracked separately in #40 per the PR description — fine to defer.


[P2] Spec re-init is duplicated three times

Where: server-context.cpp:4379-4388 (T2 rollback) and 4411-4420 (T2 success).

Extract a reinit_spec_on_ctx_change() helper that takes the new ctx_tgt and does the params_base.speculative.draft.ctx_tgt update + spec.reset() + per-slot reset. Same logic, same bug surface if one is changed — fold them.


Cross-references for the fork-side tracker

  • ddvnguyen/llama.cpp#36 (v4 design, Phase 2) — the [P0] T2 double-free is the same class of ownership-confusion bug that bit #43 (dangling hydra_rpc_ctx stack pointer). The pattern is "raw pointer that looks like an owned resource". Worth adding a note in docs/architecture.md about the llama_init → pimpl->context ownership so future forks don't repeat this.
  • ddvnguyen/llama.cpp#40 — extend the "Open design questions" with Q5: "T2 only — what does the parent do if the engine returns params_applied.n_ctx < requested?" (the silent clamp at line 4278). The C# side at hydra_vortex#407 may want to know the clamped value or treat the lower-than-requested as a 4xx.

Build/test plan check

The PR claims test-hydra-configure-tier, test-hydra-state-chunk-size, test-hydra-checkpoint-policy, test-hydra-rpc-bind all pass. None of these exercise the T2 unload → T3 reload interaction (or even T2 alone followed by shutdown). The live E2E in docs/system-test-phase-2b.md is the right place to catch the P0 — please run it once end-to-end before merging. The T2 → shutdown path will manifest as a use-after-free on the next sleep/wake or a hard crash on container stop; the T2 → T3 path will manifest as a crash on the second profile switch.


Verdict

REQUEST_CHANGES. The P0 (T2 double-free) must be fixed and a T2 → T3 + T2 → shutdown test added. The P1s are worth a commit but won't block if the P0 is fixed; P2s can be follow-up issues. Architecture-level solution (the release_context() API) is preferred over a local llama_init.reset() hack — please don't paper over this with a try/catch around the destroy path.

@hydra-z hydra-z Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review — additional findings beyond the existing CHANGES_REQUESTED

The existing review (ddv-hydra, 2026-07-11) caught the P0 T2 double-free via llama_init ownership and several P1s. Agreed on all of those. This review adds what the prior pass missed — three issues, two of them P0, found by tracing the lifetime of the staged state and the T2/T3 diff path end-to-end.


[P0] attention_type and yarn_* keys are dead code — hydra_classify_config_key doesn't recognize them

Where: tools/server/server-context.cpp:2202-2228 (the classifier) vs. apply_t2_rebuild and hydra_t2t3_key_changed.

The CONFIGURE handler at line 3180 does if (tier == 0) { continue; } — unknown keys are dropped from t2t3_subset and never reach the T2 apply path. The classifier currently recognizes only n_ctx, cache_type_k, cache_type_v, and rope_* (prefix match) as T2. attention_type, yarn_ext_factor, yarn_attn_factor, yarn_beta_fast, yarn_beta_slow, yarn_orig_ctx all return 0.

But the new code added in this PR handles all of them:

  • apply_t2_rebuild lines 4308-4327: params_base.yarn_* = cfg["yarn_*"] (5 keys) + lines 4331-4339: params_base.attention_type = ... (2 cases).
  • hydra_t2t3_key_changed lines 4167-4197: same 6 keys.

So a CONFIGURE {"yarn_ext_factor": 1.0} from the parent lands in the engine, the engine logs SRV_DBG("hydra: CONFIGURE unknown key 'yarn_ext_factor' — ignored") (line 3185), the response says deferred_keys: [], the T2 apply path never fires, and the operator's config is silently a no-op. The fix in commit 50505cf added the application side but missed the classification side. Same for the diff helper — it's never reached.

Fix: add these 6 keys to the T2 branch in hydra_classify_config_key (line 2202):

if (key == "n_ctx"        ||
    key == "cache_type_k" ||
    key == "cache_type_v" ||
    key == "attention_type" ||
    key == "yarn_ext_factor" ||
    key == "yarn_attn_factor" ||
    key == "yarn_beta_fast" ||
    key == "yarn_beta_slow" ||
    key == "yarn_orig_ctx" ||
    key.rfind("rope_", 0) == 0) {
    return 2;
}

Verification: add a test-hydra-configure-tier case that sends {"yarn_ext_factor": 1.0} and asserts deferred_keys: ["yarn_ext_factor"] and the response is tier T2. Right now the test only covers the wire response shape, not the T2 apply path, which is why this slipped through.


[P0] tensor_buft_overrides dangling pointers after the T3 path returns

Where: tools/server/server-context.cpp:4453-4542 (T3 pattern_strings + override parsing) and load_model line 844 (params_base = params;).

apply_t3_rebuild declares std::vector<std::string> pattern_strings at line 4453 (function scope, reserved to 16 to defeat SSO reallocation). It parses the wire string and stores entry.pattern = pattern_strings.back().c_str(); — raw pointers into the std::string SSO buffers. The {nullptr, nullptr} sentinel at line 4534 terminates the override list for the model loader. Then load_model(swapped_params) is called.

load_model at line 844 does params_base = params; — this copies the tensor_buft_overrides vector by value, including the C-string pointers. So params_base.tensor_buft_overrides[i].pattern now points to buffers owned by pattern_strings, which is local to apply_t3_rebuild.

apply_t3_rebuild then returns. pattern_strings is destroyed. params_base.tensor_buft_overrides[i].pattern is now dangling in params_base.

The triggering scenario is the second T3 CONFIGURE that doesn't include override_tensor:

  1. T3 #1: CONFIGURE {"override_tensor": "blk.*.ffn_*_exps.weight=CPU"} → pattern_strings created, overrides parsed, load_model runs, params_base now has dangling tensor_buft_overrides. pattern_strings destroyed. Engine stable.
  2. T3 #2: CONFIGURE {"n_gpu_layers": 40} (no override) → apply_t3_rebuild does swapped_params = params_base; at line 4446, which copies the dangling pointers. if (override && *override) is false (no pending override), so the parsing block is skipped. load_model(swapped_params) runs → model loader dereferences swapped_params.tensor_buft_overrides[0].patternuse-after-free / SIGSEGV / undefined behavior.

The PR's comment at lines 4441-4452 correctly identifies the load_model lifetime requirement but does not account for the copy-into-params_base semantics.

Fix (smallest): after load_model returns successfully, clear params_base.tensor_buft_overrides:

if (!load_model(swapped_params)) { ...rollback... }
// load_model() has finished using the override patterns; the
// vector in params_base now holds dangling C-string pointers
// (the std::string storage was local to apply_t3_rebuild).
// Clear it so the next apply_t3_rebuild doesn't copy stale pointers.
params_base.tensor_buft_overrides.clear();

Fix (cleaner): copy patterns into the tensor_buft_overrides storage's lifetime. Easiest is to make the pattern strings a server_context member:

// In server_context:
std::vector<std::string> hydra_pending_override_patterns;
// In apply_t3_rebuild success: copy the patterns to a server_context
// member that outlives any single apply call.
hydra_pending_override_patterns = pattern_strings;

Either way, the load_model signature is a footgun that needs a comment, or it should take overrides as a separate argument that's clear at the call site that the lifetime ends when load_model returns.


[P1] common_speculative_init is called without try/catch in T2 paths

Where: server-context.cpp:4413 (T2 success) and server-context.cpp:4388 (T2 rollback).

The original load_model() at lines 1136-1143 has:

try {
    spec.reset(common_speculative_init(params_base.speculative, params_base.n_parallel));
} catch (const std::exception & e) {
    SRV_ERR("failed to initialize speculative decoding context: %s\n", e.what());
}

The T2 path does NOT have the try/catch:

spec.reset(common_speculative_init(params_base.speculative, params_base.n_parallel));

common_speculative_impl_draft_simple (common/speculative.cpp:229) throws std::runtime_error on vocab mismatch. common_speculative_impl_draft_mtp (line 445) GGML_ASSERTs on null ctx_tgt / ctx_dft (which would abort). If the T2 path is reachable with MTP active (it is — the slot-free trigger doesn't care about spec), an unhandled exception propagates out of apply_t2_rebuildapply_pending_hydra_configupdate_slots, killing the request handler thread. Spec re-init failure should be logged and continued (matching load_model's behavior), not fatal.

The rollback path (line 4388) has the same issue. The pre-fix b915e0df and 2600bef44 commits added spec re-init on both paths but missed the try/catch.

Fix: wrap both spec.reset(common_speculative_init(...)) calls in try/catch, matching load_model line 1138-1142.


[P2] n_ctx member not updated on T2 rollback — stale metrics

Where: server-context.cpp:4351-4353 (rollback path) vs. 4422 (success path).

Success path: n_ctx = llama_n_ctx(ctx_tgt); at line 4422.

Rollback path returns at line 4398 without updating the n_ctx member. The rollback restored an old ctx_tgt (which may have a different n_ctx than the post-rebuild value) and the member is now stale. n_ctx is consumed in:

  • n_ctx JSON field at line 545 ({"n_ctx", n_ctx} in metrics responses).
  • prompt_cache sizing at line 1205 (only at init — not relevant here, but illustrative).

For the metrics field, an operator querying after a failed T2 CONFIGURE will see the post-rebuild n_ctx even though the engine is back on the old one. Minor but misleading.

Fix: add n_ctx = llama_n_ctx(ctx_tgt); before the SRV_INF("hydra: T2 rollback succeeded...") log at line 4396.


[P2] Drain timeout uses signed-unsigned arithmetic — clock skew / time_t wrap can bypass the timeout

Where: server-context.cpp:4089:

time_t now = std::time(nullptr);
time_t elapsed = now - ctx_tgt->hydra_get_pending_config_set_at();

time_t is implementation-defined (signed on glibc x86-64, unsigned elsewhere). If now < set_at (clock skew between the CONFIGURE time and the slot-free moment, or monotonic wrap), elapsed is negative and the if (elapsed > drain_timeout) check passes (negative > positive is false), so the timeout is silently bypassed. With a 300s default and a clock-correct system this is unlikely; with a clock-uncorrected system or a long-running container, it can happen.

Fix: cast to int64_t for the subtraction and compare with (int64_t) drain_timeout:

const int64_t now     = (int64_t) std::time(nullptr);
const int64_t set_at  = (int64_t) ctx_tgt->hydra_get_pending_config_set_at();
const int64_t elapsed = now - set_at;
if (elapsed > (int64_t) drain_timeout) { ... }

Verdict

REQUEST_CHANGES. The P0s (dead-code tier classification, dangling tensor_buft_overrides in params_base) compound the existing P0 (T2 double-free) — three independent ways for the engine to crash on a T2 or T3 CONFIGURE. The P1 (spec re-init exception) is a one-line fix that should land in the same commit.

The structural direction is right (slot-free trigger, T2/T3 separation, COMBINED reattach, rollback path) and the in-tree review iterations (b915e0d / 2600bef / 50505cf / 143a25b / b5e4652 / c184c4f) are addressing real bugs. The dead-code and dangling-pointer issues are exactly the class of bug the test suite needs to catch — please add a test-hydra-configure-apply test that exercises:

  • T2 with yarn_ext_factor (catches P0 #1)
  • T2 → T3 sequence (catches the existing P0 double-free + the new P0 dangling-pointer on T3)
  • T2 with MTP active and a deliberately-bad params_base.speculative (catches P1)
  • T2 CONFIGURE where the second llama_new_context_with_model fails (catches rollback path coverage)

before requesting review re-pass. The current test-hydra-configure-tier only validates JSON shape, not the apply path.

@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 — T2/T3 apply path + 4 follow-up commits

Picking up from ddv-hydra[bot]'s review and the 4 follow-up commits. Most of the high-severity findings are already addressed; here is the residual from a fresh pass against e22aaed3.

P1 — T3 teardown breaks layer-split COMBINED reloads

apply_t3_rebuild() at tools/server/server-context.cpp:4542-4551 tears down the peer unconditionally when was_combined is true. For hydra_combined_static (layer-split, DENSE profile) this is destructive:

  1. ctx_tgt->hydra_remove_combined_rpc_backend(hydra_current_peer.c_str()) calls ggml_backend_rpc_remove_server(endpoint) at src/llama-context.cpp:499 — the peer is unregistered from the ggml registry.
  2. load_model(swapped_params) then runs. The new llama_model::devices is built by llama_prepare_model_devices() iterating ggml_backend_dev_count() (src/llama.cpp:126). The peer is no longer there, so the new model is placed across the local device(s) only — the tensor_split ratio for the peer is silently absorbed.
  3. The post-load reattach for layer-split (server-context.cpp:4614-4616) only calls llama_hydra_set_expert_mode(ctx_tgt, 1). That sets a runtime flag — it does not re-add the peer to the backends vector.

Result: after a T3 reload on a DENSE engine, the engine silently drops the 3060 half of the model onto the 5060 Ti's VRAM, then OOMs on the next prefill. The "combined" mode bit is set, so the Coordinator still sees it as combined — masking the regression.

Repro path: dense-profile engine running across 5060 Ti + 3060 with --tensor-split, trigger any T3 (e.g. n_gpu_layers change), observe OOM on next prefill.

Fix: gate the teardown. For layer-split, only zero the mode flag — leave the peer in place so the model loader sees it on the next load_model():

if (was_combined) {
    llama_hydra_set_expert_mode(ctx_tgt, 0);
    if (!hydra_combined_static) {
        // expert-split only: the new model will be loaded without
        // the peer's runtime-bound expert tensors. For layer-split,
        // the model loader needs the peer registered to do the
        // layer placement, so we must NOT remove it here.
        if (!hydra_current_peer.empty()) {
            ctx_tgt->hydra_remove_combined_rpc_backend(hydra_current_peer.c_str());
        }
        llama_hydra_clear_combined_bindings(ctx_tgt, hydra_peer.c_str());
        hydra_combined_head_attached = false;
    }
}

The rollback path at server-context.cpp:4576-4604 has the same bug — load_model(old_params) runs after the peer is gone, and the hydra_combined_static reattach branch re-enables the flag without re-registering. Apply the same gate to the rollback block.

P2 — hydra_current_peer vs hydra_peer divergence in teardown

server-context.cpp:4547-4550:

ctx_tgt->hydra_remove_combined_rpc_backend(hydra_current_peer.c_str());  // current
llama_hydra_clear_combined_bindings(ctx_tgt, hydra_peer.c_str());         // configured

Two different variables. Today they are kept in sync (per the comment added in cac7122a), but the moment SET_EXPERT_MODE does a per-request peer switch (the explicit #29 design, server-context.cpp:3910-3932), this tears down the wrong binding. Pick one consistently — verify which key the binding map actually uses, then use that for both.

P2 — T2 success/rollback spec reinit blocks are duplicated verbatim

server-context.cpp:4379-4388 and 4411-4420 are byte-for-byte identical 10-line blocks. This was the exact bug class that commit 2600bef4 fixed on the rollback path (it was missing in an earlier version). Extract into a private helper:

void refresh_spec_after_ctx_rebuild() {
    if (!spec) return;
    params_base.speculative.draft.ctx_tgt = ctx_tgt;
    spec.reset(common_speculative_init(params_base.speculative, params_base.n_parallel));
    for (auto & slot : slots) {
        slot.spec = spec ? spec.get() : nullptr;
        slot.spec_ckpt.clear();
        slot.spec_draft.clear();
        slot.spec_i_batch.clear();
    }
}

Same logic for the per-slot smpl/ctx_tgt/n_ctx refresh — also duplicated at 4371-4375 and 4400-4404.

P3 — Dead drain-timeout in llama_hydra_apply_pending_config() stub

src/llama-hydra.cpp:536-585. The PR's own comment at server-context.cpp:4680-4682 notes the stub is now a no-op for the server caller. The drain-timeout inside the stub is unreachable. Either remove the stub or mark it [deprecated] and move the timeout logic out (it's already in apply_pending_hydra_config() at server-context.cpp:4081-4094).

P3 — s_hydra_pending_* thread-model assumption is fragile

src/llama-hydra.cpp:463-468 and the new thread-invariance comment at 453-462. The invariant (CONFIGURE-handler and update_slots must run on the same thread) is correct but not enforced — it depends on the bounded-thread-pool's current dispatch behavior, which is one-line-change away from breaking. The comment itself suggests moving these into llama_context (which already owns hydra_pending_config_json); this PR is the right time to do it, before more consumers accumulate.

P3 — apply_t3_rebuild blocks the serving loop synchronously

The PR's own comment at server-context.cpp:4561-4565 documents this. A 35B GGUF reload can take 30+ seconds; the drain-timeout only guards entry, not the reload. A background-thread offload with a "draining, no new requests" gate is a real improvement but correctly deferred.

Test gap

test-hydra-configure-tier passing does not exercise a layer-split T3 reload. Add a test-hydra-configure-layer-split case that:

  1. Starts a dense-profile engine with --tensor-split set
  2. Verifies pre-reload that model->devices includes the peer
  3. Triggers a T3 (e.g. n_gpu_layers change)
  4. Post-reload asserts: model->devices still includes the peer, the peer's RPC backend is still registered (ggml_backend_dev_count()), and the scheduler has the peer backend

This test would have caught the P1 above on the first CI run.

Verified correct

  • params_base = params at load_model() line 844 — relying on this side effect is sound.
  • tensor_buft_overrides null-termination matches the iterator at src/llama-model-loader.cpp:1158 (overrides->pattern != nullptr).
  • hydra_parse_cache_type() whitelist matches upstream's kv_cache_types at common/arg.cpp:391-401 (F32, F16, BF16, Q8_0, Q4_0, Q4_1, IQ4_NL, Q5_0, Q5_1).
  • T2 spec reinit at 4411-4413 correctly handles the impls' copied ctx_tgt pointer (common/speculative.cpp:177,439 confirm copy-by-value).
  • pattern_strings.reserve(16) correctly prevents SSO reallocation of c_str() pointers in entry.pattern.
  • set_hydra_combined_static(true) (commit 143a25b23) is the correct entry-point flag for DENSE layer-split.
  • The hydra_rpc_ctx static-storage fix (c184c4fb) correctly identifies the stack-use-after-return.
  • The hydra_recv_all EINTR/EAGAIN retry (b5e46527) correctly distinguishes signal-induced transients from a real SO_RCVTIMEO idle timeout.
  • The engine-side config diff (e22aaed3) is a clean optimization — moving the drain decision engine-side is the right ownership split.

Recommendation

Request changes for the P1 (layer-split T3 teardown). The P2s and P3s are worth addressing but not blocking. The P1 is a silent correctness regression that the current test suite cannot detect.

ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…buft_overrides

load_model(swapped_params) copies params into params_base (line 844),
which includes tensor_buft_overrides entries whose entry.pattern is a
raw const char* into pattern_strings. If pattern_strings is a local
variable in apply_t3_rebuild(), it is destroyed at function exit,
leaving dangling pointers in params_base.tensor_buft_overrides.

Move pattern_strings to server_context_impl::hydra_pattern_strings
(member scope) so it outlives apply_t3_rebuild(). Cleared explicitly
on each T3 apply.

Refs: #42 (follow-up review P0)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…it helpers

P1: T3 teardown breaks layer-split COMBINED reloads. For
hydra_combined_static (layer-split, DENSE profile), the teardown
removes the peer from the ggml backend registry. Then load_model()
builds the new model without the peer, placing the entire model on
the local GPU — OOM on next prefill. The reattach only sets a flag
without re-registering the peer.

Fix: gate the teardown on hydra_combined_static. For layer-split,
only zero the mode flag — leave the peer registered so load_model()
can see it during tensor_split placement. For expert-split, the full
teardown is required.

P2: Consolidate hydra_current_peer vs hydra_peer in teardown. Use
hydra_current_peer consistently for both remove and clear — it's the
endpoint actually registered in the ggml backend registry (SET_EXPERT_MODE
registers via hydra_current_peer). The reattach paths correctly use
hydra_peer for re-registration.

P2: Extract refresh_slots_after_ctx_rebuild() helper. Both the success
and rollback paths of apply_t2_rebuild had byte-for-byte identical
15-line blocks (slot pointers + smpl + spec reinit). This was the
exact bug class where the rollback path originally missed the spec
reinit. Single helper eliminates the possibility of recurrence.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
n_cpu_moe: always return true (no params equivalent; informational
only — apply path just logs it).

model.path: fall back to params.model.path when pending static is
empty, so re-sending the current path doesn't trigger a spurious
T3 reload (same pattern as n_gpu_layers).

override_tensor: always return true (no params equivalent; apply
path parses and applies on model reload).

Refs: #42

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

Copy link
Copy Markdown
Owner Author

Re-review of the 4 new cleanup commits

Follow-up to the earlier review. All 4 commits verified against the diff.

Confirmed fixed:

  • f266e6787 — override_tensor pattern lifetime. Member-scope hydra_pattern_strings survives load_model()'s params_base = params copy. ✅
  • 02c7a9d99 — P1 layer-split teardown OOM (was my miss), P2 spec re-init extracted into refresh_slots_after_ctx_rebuild(), P2 peer consistency. All three solid. ✅
  • 8e0122e15 — T3 statics moved to llama_context. Threading invariant gone, future concurrent contexts safe. ✅
  • 3f87a7b51 — diff check false positives after drain. n_cpu_moe / override_tensor / model.path fall-back to params.model.path when the pending static is empty. ✅

Optional cleanup before merge (non-blocking)

P3 — refresh_slots_after_ctx_rebuild uses the wrong slot.n_ctx formula

tools/server/server-context.cpp:4147:

const int32_t n_ctx_slot = llama_n_ctx(ctx_tgt) / params_base.n_parallel;

load_model() itself uses llama_n_ctx_seq(ctx_tgt) (line 1122), which reads cparams.n_ctx_seq = GGML_PAD(n_ctx / n_seq_max, 256) (src/llama-context.cpp:213-214). Two discrepancies:

  • T2 uses n_parallel, load_model uses n_seq_max (rare for these to differ, but possible after a n_parallel change that didn't go through load_model).
  • T2 omits the GGML_PAD(..., 256) alignment, so slot.n_ctx can disagree with what a fresh load_model would have set — undercount when the raw division isn't a multiple of 256, which would make context-shift/truncation trigger earlier than it should.

One-line fix:

const int32_t n_ctx_slot = llama_n_ctx_seq(ctx_tgt);

Narrow trigger (only matters if n_seq_max != n_parallel or the alignment is off), safe to fix in a follow-up if not in this PR.


P0 (latent) — llama_hydra_get_pending_config_tier still returns a dangling pointer

src/llama-hydra.cpp:578-581:

const char * llama_hydra_get_pending_config_tier(const struct llama_context * ctx) {
    if (!ctx) return "";
    return ctx->hydra_get_pending_config_tier().c_str();   // c_str() of a temporary
}

hydra_get_pending_config_tier() returns std::string by value; the temporary is destroyed at the end of the full expression, so the returned c_str() dangles before any caller dereferences it. This is the same bug PR #41 review (baf6c93bb) explicitly fixed with thread_local static std::string s_cached_tier; — and cac7122a3 (the first review-fix commit on this PR) silently undid that fix.

Impact today: zero. git grep confirms there are no in-tree callers — every consumer in tools/server/server-context.cpp and the apply step itself uses ctx->hydra_get_pending_config_tier() directly or copies into a local std::string. The wrapper is effectively dead code.

Why it still matters: the function is LLAMA_API-exported (include/llama-hydra.h:43) and the header comment documents an intent ("Used by the INFO response to advertise 'drain in progress'"). Any future external consumer (the C# RpcClient, the future ProfileSwitcher observability, etc.) will hit a use-after-return. The comment is also stale — no INFO response actually calls this.

Three options, in increasing order of disruption:

  1. Restore the thread_local static from baf6c93bb. 4-line change, preserves the API as documented, fixes the bug. Preferred if you want zero behavior change.
  2. Change the signature to return const std::string & — requires llama_context to keep a std::string member instead of returning by value, slightly more refactor.
  3. Delete the function entirely. Cleanest, but the API in the public header would shrink. Safe if you confirm no external consumer; the C# side talks via the wire protocol and doesn't import the C header.

I'd suggest option 1 if you want a clean ship, or option 3 if you want to remove dead code.


Follow-up issue (separate from this PR)

P1 (deferred) — background-thread T3 reload

apply_t3_rebuild() runs synchronously inside update_slots(). A full model reload (GGUF read + VRAM alloc/free) blocks the entire serving loop for the entire reload. The drain-timeout env var only guards entry, not the reload's own duration. For the headline use case (MoE↔DENSE profile switch) this is multi-second blocking; the engine appears hung to the Coordinator with no progress signal.

8e0122e15's commit message correctly identifies that the fix needs double-buffering (because load_model() replaces nearly every member variable). That's an architectural change, not appropriate for this PR.

Will file as review-finding in ddvnguyen/llama.cpp, cross-link to ddvnguyen/hydra_vortex#397.


Cross-repo coordination note

ddvnguyen/hydra_vortex#407 currently has the submodule at cac7122a3 (the first review-fix commit on this PR). It does not include the 4 new cleanup commits. The C# side doesn't call any of the changed C APIs directly (it talks via the wire protocol), so ggml-org#407's C# code still compiles against the new submodule pointer, but:

  • After this PR merges to hydra-fork, Add support for batch size to --perplexity ggml-org/llama.cpp#407 will need a follow-up bump to at least 3f87a7b51 (and ideally e22aaed38 for the diff feature) before it can land on main. The push-before-PR guard in docs/workflow/02-implement.md / 04-commit-pr.md already enforces this.
  • The 8e0122e15 API change (the T3 mutator getters/setters now take a llama_context* parameter) is a wire-incompatible C API change. Any future Hydra-side C bindings (none today) would need to be regenerated.

Heads-up only; not blocking.


Verdict

APPROVE the 4 cleanup commits. Optional before merge: fix the P0 dangling pointer (option 1 or 3 above) and the P3 slot.n_ctx formula. File the background-thread T3 reload as a review-finding follow-up.

@ddvnguyen ddvnguyen force-pushed the fork/phase-2b-t2t3-apply branch from 3f87a7b to 5ab801c Compare July 12, 2026 03:46
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
Addresses all review findings from the ddv-hydra review bot on
#41 and #42.

## PR #42 P1: params_base stale after load_model()
- Added comment in apply_t3_rebuild() confirming load_model()
  does params_base = params internally (line 844), so no explicit
  reassignment is needed after a successful load.

## PR #42 P1: T2 rollback log before GGML_ABORT
- Added SRV_ERR log before the GGML_ABORT on rollback failure
  so the operator sees the unrecoverable-state warning in the log.
- Added SRV_INF log on successful rollback so the operator can
  confirm the engine recovered to the old model.

## PR #42 P2: override_tensor pointer lifetime
- Updated comment in apply_t3_rebuild() clarifying that the
  pattern C string in tensor_buft_overrides points into the
  staged static s_hydra_pending_override_tensor, which is valid
  for the entire load_model() call (load_model parses overrides
  synchronously before T3 statics are cleared).

## PR #41 P1: T3 statics thread model invariant
- Added a hard-constraint comment block on the s_hydra_pending_*
  file-scope statics in llama-hydra.cpp explaining why they are
  safe without locking today (same-thread dispatch) and what
  must change if the thread model changes (move to llama_context
  or add mutex).

## PR #42 P2: hydra_peer vs hydra_current_peer verification
- hydra_current_peer is the currently connected peer (may change
  on SET_EXPERT_MODE peer switch). hydra_peer is the configured
  peer (from startup YAML). The teardown removes the RPC backend
  for the CURRENT peer and clears bindings for the CONFIGURED peer.
  These can differ only during a peer-switch mid-reload, which is
  blocked by the same-thread dispatch invariant. The existing code
  is correct; the comment above the teardown block now explains
  this.

## PR #42 P3: dead drain timeout in llama-hydra.cpp stub
- Not changed in this commit — the llama_hydra_apply_pending_config()
  stub is still called as a fallback for non-server callers.
  Left as-is for now; can be removed in a follow-up.

Refs: #41 (review), #42 (review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
Addresses all findings from the ddvnguyen review (Jul 11, 2026) on
#42:

1. [P0] stale slot.ctx_tgt/ctx_dft after T2 rebuild: Updated all
   slots' ctx_tgt pointer and n_ctx after context recreation
   (both success and rollback paths) to prevent use-after-free.

2. [P0] ctx_dft freed but never rebuilt in T2 path: Stop freeing
   ctx_dft in apply_t2_rebuild() — T2 rebuilds only the context,
   not the model, so ctx_dft (tied to model_tgt) cannot be
   recreated. Keeping it alive preserves MTP/speculative decode.

3. [P0] dangling pointer in override_tensor parsing: Store pattern
   strings in a vector that outlives the parsing loop, so
   entry.pattern C strings remain valid when load_model() reads
   them.

4. [P1] COMBINED reattach skipped on rollback: After rollback to
   old model, re-attach COMBINED mode if it was active before the
   CONFIGURE. Without this, the engine silently stays in SOLO.

5. [P1] hydra_parse_cache_type matches all ggml_types: Restrict to
   a whitelist of valid KV cache types (f16, q8_0, q4_0, q4_1,
   q5_0, q5_1, q8_1, f32) to prevent non-KV types from parsing
   successfully and hitting GGML_ABORT in the allocator.

6. [P2] slot.n_ctx never refreshed after T2 n_ctx change: Fixed as
   part of #1 — slot.n_ctx is now updated after both success and
   rollback.

7. [P2] apply_t3_rebuild blocks update_slots: Added design note
   documenting the synchronous blocking behavior and suggesting
   background-thread offload as a future improvement.

8. [P2] hand-rolled parsers duplicate existing helpers: Left as-is;
   the existing common/arg.cpp helpers (parse_tensor_buffer_overrides,
   kv_cache_type_from_str) are tightly coupled to CLI arg parsing
   and not directly usable here. Refactor tracked in #40.

Refs: #42

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…42

Addresses the residual issues identified in the Jul 11 follow-up
review comments:

1. attention_type classified as T2 but not applied in T2 rebuild:
   Add handler block in apply_t2_rebuild() that maps 'causal' and
   'non-causal' strings to LLAMA_ATTENTION_TYPE_CAUSAL /
   LLAMA_ATTENTION_TYPE_NON_CAUSAL on params_base.attention_type.
   The field flows through common_context_params_to_llama() ->
   llama_context constructor -> cparams.causal_attn.

2. Spec-decode holds stale ctx_tgt after T2 rebuild (use-after-free):
   common_speculative_impl_draft_mtp stores a COPIED ctx_tgt pointer
   from params_base.speculative.draft.ctx_tgt at init time. After
   T2 rebuilds the context, this pointer is dangling. Fix: update
   params_base.speculative.draft.ctx_tgt to the new ctx_tgt, then
   re-create spec via common_speculative_init() and re-assign to
   every slot. Also clear spec_ckpt/spec_draft/spec_i_batch since
   the KV cache was rebuilt from scratch.

3. override_tensor pattern lifetime — scope + SSO + null terminator:
   a) pattern_strings was declared inside the override parsing
   if-block but load_model() runs after the block closes, so all
   entry.pattern C strings were dangling. Move declaration to
   function scope alongside swapped_params.
   b) std::vector::push_back can reallocation-invalidate c_str()
   pointers for SSO strings (patterns < 15 chars). Add reserve(16)
   to prevent reallocation.
   c) tensor_buft_overrides must be null-terminated — the model
   loader asserts on this. Add {nullptr, nullptr} sentinel.

4. Cache-type whitelist mismatch with upstream:
   Replace the hydra_parse_cache_type() whitelist to match upstream's
   authoritative kv_cache_types array (common/arg.cpp:390): add
   GGML_TYPE_BF16 and GGML_TYPE_IQ4_NL, remove GGML_TYPE_Q8_1.

Refs: #41 (follow-up review), #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…fresh)

The previous commit added spec re-init on apply_t2_rebuild's success
path, but the rollback path (first context-recreate fails, falls back
to old params) also creates a fresh ctx_tgt without re-initing spec.
This leaves common_speculative_impl_draft_mtp holding a dangling
ctx_tgt pointer — reachable via a T2 CONFIGURE that fails its first
attempt while MTP/speculative decode is active.

Mirror the same if (spec) { ... } re-init block into the rollback
branch.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…ent state

The engine owns the drain decision, not Hydra Core. Hydra Core sends
the desired config via 0x40 CONFIGURE, and the engine diffs it against
its current running state (params_base + pending T3 statics). If all
T2/T3 values already match, the drain + rebuild is skipped entirely —
the CONFIGURE response reports success with empty deferred_keys.

Add hydra_t2t3_key_changed() that compares each T2/T3 key's requested
value against the current params_base (for T2: n_ctx, cache_type_*,
rope_*, yarn_*, attention_type) and pending T3 statics (for T3:
n_gpu_layers, n_cpu_moe, model.path, override_tensor). Keys that
match are filtered out before staging. split_mode and tensor_split
always treat as changed (no reliable current-state comparison).

Refs: #42

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…buft_overrides

load_model(swapped_params) copies params into params_base (line 844),
which includes tensor_buft_overrides entries whose entry.pattern is a
raw const char* into pattern_strings. If pattern_strings is a local
variable in apply_t3_rebuild(), it is destroyed at function exit,
leaving dangling pointers in params_base.tensor_buft_overrides.

Move pattern_strings to server_context_impl::hydra_pattern_strings
(member scope) so it outlives apply_t3_rebuild(). Cleared explicitly
on each T3 apply.

Refs: #42 (follow-up review P0)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
…it helpers

P1: T3 teardown breaks layer-split COMBINED reloads. For
hydra_combined_static (layer-split, DENSE profile), the teardown
removes the peer from the ggml backend registry. Then load_model()
builds the new model without the peer, placing the entire model on
the local GPU — OOM on next prefill. The reattach only sets a flag
without re-registering the peer.

Fix: gate the teardown on hydra_combined_static. For layer-split,
only zero the mode flag — leave the peer registered so load_model()
can see it during tensor_split placement. For expert-split, the full
teardown is required.

P2: Consolidate hydra_current_peer vs hydra_peer in teardown. Use
hydra_current_peer consistently for both remove and clear — it's the
endpoint actually registered in the ggml backend registry (SET_EXPERT_MODE
registers via hydra_current_peer). The reattach paths correctly use
hydra_peer for re-registration.

P2: Extract refresh_slots_after_ctx_rebuild() helper. Both the success
and rollback paths of apply_t2_rebuild had byte-for-byte identical
15-line blocks (slot pointers + smpl + spec reinit). This was the
exact bug class where the rollback path originally missed the spec
reinit. Single helper eliminates the possibility of recurrence.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit that referenced this pull request Jul 12, 2026
n_cpu_moe: always return true (no params equivalent; informational
only — apply path just logs it).

model.path: fall back to params.model.path when pending static is
empty, so re-sending the current path doesn't trigger a spurious
T3 reload (same pattern as n_gpu_layers).

override_tensor: always return true (no params equivalent; apply
path parses and applies on model reload).

Refs: #42

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hydra Engineering and others added 13 commits July 12, 2026 10:49
…lot-free moment

Phase 2b of #36 — follow-up to #41.

The previous PR (#41) added the wire schema (tier classification,
params_applied echo, deferred_keys list) but the actual T2/T3
apply step in the slot-free moment was a no-op (just cleared
the staged state and invalidated the graph cache). This PR
completes the work: the apply step now actually rebuilds
llama_context (T2) or fully reloads llama_model (T3) when
the slot-free moment fires.

## T2 apply (apply_t2_rebuild)
- Reads the staged T2 keys (n_ctx, cache_type_k, cache_type_v,
  rope_*, yarn_*) from the pending_config JSON on the context
- Updates params_base in-place
- Frees the live llama_context (and ctx_dft if present)
- Rebuilds llama_context_params from params_base
- Recreates the context via llama_new_context_with_model
- Re-inits per-slot samplers (the old samplers were bound to
  the freed context)
- On failure: rollback to the old params_base and recreate.
  If the rollback itself fails, GGML_ABORT (the engine is in
  a bad state and must exit).

## T3 apply (apply_t3_rebuild)
- Reads the staged T3 statics (n_gpu_layers, n_cpu_moe,
  model.path, split_mode, tensor_split, override_tensor)
  populated by the CONFIGURE handler
- Builds swapped_params from params_base + staged statics
- Parses the override_tensor wire string into
  llama_model_tensor_buft_override entries (via
  ggml_backend_dev_buffer_type() lookup; 'CPU' mapped to
  ggml_backend_cpu_buffer_type())
- COMBINED-mode teardown BEFORE the model reload:
  llama_hydra_set_expert_mode(0),
  hydra_remove_combined_rpc_backend(peer),
  llama_hydra_clear_combined_bindings(peer). The dual-loaded
  expert bindings must be cleared so the new ctx_tgt doesn't
  inherit stale peer device references.
- Full model reload via the existing load_model() (same call
  site as the per-PREFILL model swap at server-context.cpp:3392).
  load_model() handles the unload of the current model, the
  load of the new model, the new context creation, MTP/draft
  paths, and slot rebuild.
- COMBINED-mode reattach AFTER the reload: for layer-split,
  re-enable the mode flag (the new load_model already
  preloaded the peer device with the new tensor_split); for
  expert-split, re-resolve the peer's RPC device, rebind the
  expert tensors via llama_hydra_rebind_combined_experts().
  Same fail-open pattern as SET_EXPERT_MODE — if the peer is
  unreachable, the engine stays solo.
- On failure: rollback by reloading the old params_base.
  If the rollback itself fails, GGML_ABORT.

## Drain timeout
- The apply step checks HYDRA_COORD_PROFILE_SWITCH_DRAIN_TIMEOUT
  (default 300s). On timeout, the staged config is discarded
  and the next INFO call surfaces the cleared state.

## Files

- tools/server/server-context.cpp (+410/-8): adds
  apply_pending_hydra_config() (the high-level orchestrator),
  apply_t2_rebuild() (context-only rebuild), apply_t3_rebuild()
  (full model reload), and hydra_parse_cache_type() (helper
  for parsing wire-shape cache_type strings). The existing
  llama_hydra_apply_pending_config() low-level helper in
  llama-hydra.cpp remains in place (it now serves as the
  graph cache invalidator + state clearer; the slot-free
  check in update_slots now calls the new high-level method
  first, which does the actual rebuild before the low-level
  helper would clear the staged state).

## Build status

- sm_120 (RTX 5060 Ti): builds clean
- sm_60 (Tesla P100): builds clean (with CMAKE_CUDA_HOST_COMPILER=g++-14)
- Tests: test-hydra-state-chunk-size, test-hydra-configure-tier,
  test-hydra-checkpoint-policy, test-hydra-rpc-bind — all pass

## Cross-references

- #36 (v4 design handoff, Phase 2)
- #40 (fork-side Phase 2b issue)
- #41 (predecessor PR — wire schema + best-effort stub)
- ddvnguyen/hydra_vortex#397 (parent tracker)
- ddvnguyen/hydra_vortex#406 (parent-side docs PR — the wire schema)
- ddvnguyen/hydra_vortex#407 (parent-side code PR — applier + overrides)
- docs/system-test-phase-2b.md (live E2E test procedure)

Refs: #41, ddvnguyen/hydra_vortex#406,
ddvnguyen/hydra_vortex#407

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses all review findings from the ddv-hydra review bot on
#41 and #42.

## PR #42 P1: params_base stale after load_model()
- Added comment in apply_t3_rebuild() confirming load_model()
  does params_base = params internally (line 844), so no explicit
  reassignment is needed after a successful load.

## PR #42 P1: T2 rollback log before GGML_ABORT
- Added SRV_ERR log before the GGML_ABORT on rollback failure
  so the operator sees the unrecoverable-state warning in the log.
- Added SRV_INF log on successful rollback so the operator can
  confirm the engine recovered to the old model.

## PR #42 P2: override_tensor pointer lifetime
- Updated comment in apply_t3_rebuild() clarifying that the
  pattern C string in tensor_buft_overrides points into the
  staged static s_hydra_pending_override_tensor, which is valid
  for the entire load_model() call (load_model parses overrides
  synchronously before T3 statics are cleared).

## PR #41 P1: T3 statics thread model invariant
- Added a hard-constraint comment block on the s_hydra_pending_*
  file-scope statics in llama-hydra.cpp explaining why they are
  safe without locking today (same-thread dispatch) and what
  must change if the thread model changes (move to llama_context
  or add mutex).

## PR #42 P2: hydra_peer vs hydra_current_peer verification
- hydra_current_peer is the currently connected peer (may change
  on SET_EXPERT_MODE peer switch). hydra_peer is the configured
  peer (from startup YAML). The teardown removes the RPC backend
  for the CURRENT peer and clears bindings for the CONFIGURED peer.
  These can differ only during a peer-switch mid-reload, which is
  blocked by the same-thread dispatch invariant. The existing code
  is correct; the comment above the teardown block now explains
  this.

## PR #42 P3: dead drain timeout in llama-hydra.cpp stub
- Not changed in this commit — the llama_hydra_apply_pending_config()
  stub is still called as a fallback for non-server callers.
  Left as-is for now; can be removed in a follow-up.

Refs: #41 (review), #42 (review)

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

server_context::start_rpc_server() declared hydra_rpc_ctx as an
automatic-storage local, then handed hydra_rpc::start() its address to
retain in a process-lifetime singleton (hydra_rpc::state().hydra_ctx),
read by every future RPC connection from a bounded-thread-pool worker
thread. The local went out of scope the moment start_rpc_server()
returned, so the singleton retained a dangling pointer into a freed
stack frame — a stack-use-after-return. Whether a given request's
queue_tasks/queue_results dereference through it "worked" depended
entirely on whether the freed stack slot had been reused yet, which
explains the intermittent nature of #43 (task processed, result
queued, but the response body never reaching the client socket).

Fix: give ctx static storage duration. start_rpc_server() only ever
runs meaningfully once per process (hydra_rpc::start() itself guards
double-start), so `static` gives it exactly the lifetime the singleton
already assumes.

Also make hydra_send_all/hydra_recv_all log on failure (fd, bytes
transferred, errno) instead of silently returning false — every caller
already treats a false return as "give up" but had no way to tell a
real socket error from `-lv` output, which the issue itself flagged as
a diagnostic gap.

Verified live on hardware (RTX 5060 Ti, Qwen3.5-2B-Q8_0): INFO (0x41)
and CONFIGURE (0x40) previously timed out after 10s against a raw
socat client, a raw Python client, and C# RpcClient. All three now
round-trip correctly, including a second request on the same
persistent connection. test-hydra-rpc-bind, test-hydra-configure-tier,
and dotnet Tests.Shared EngineOpcodeTests all still pass.

Closes #43

Co-authored-by: Hydra Engineering <hydra-engineering@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Addresses all findings from the ddvnguyen review (Jul 11, 2026) on
#42:

1. [P0] stale slot.ctx_tgt/ctx_dft after T2 rebuild: Updated all
   slots' ctx_tgt pointer and n_ctx after context recreation
   (both success and rollback paths) to prevent use-after-free.

2. [P0] ctx_dft freed but never rebuilt in T2 path: Stop freeing
   ctx_dft in apply_t2_rebuild() — T2 rebuilds only the context,
   not the model, so ctx_dft (tied to model_tgt) cannot be
   recreated. Keeping it alive preserves MTP/speculative decode.

3. [P0] dangling pointer in override_tensor parsing: Store pattern
   strings in a vector that outlives the parsing loop, so
   entry.pattern C strings remain valid when load_model() reads
   them.

4. [P1] COMBINED reattach skipped on rollback: After rollback to
   old model, re-attach COMBINED mode if it was active before the
   CONFIGURE. Without this, the engine silently stays in SOLO.

5. [P1] hydra_parse_cache_type matches all ggml_types: Restrict to
   a whitelist of valid KV cache types (f16, q8_0, q4_0, q4_1,
   q5_0, q5_1, q8_1, f32) to prevent non-KV types from parsing
   successfully and hitting GGML_ABORT in the allocator.

6. [P2] slot.n_ctx never refreshed after T2 n_ctx change: Fixed as
   part of #1 — slot.n_ctx is now updated after both success and
   rollback.

7. [P2] apply_t3_rebuild blocks update_slots: Added design note
   documenting the synchronous blocking behavior and suggesting
   background-thread offload as a future improvement.

8. [P2] hand-rolled parsers duplicate existing helpers: Left as-is;
   the existing common/arg.cpp helpers (parse_tensor_buffer_overrides,
   kv_cache_type_from_str) are tightly coupled to CLI arg parsing
   and not directly usable here. Refactor tracked in #40.

Refs: #42

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

Addresses the residual issues identified in the Jul 11 follow-up
review comments:

1. attention_type classified as T2 but not applied in T2 rebuild:
   Add handler block in apply_t2_rebuild() that maps 'causal' and
   'non-causal' strings to LLAMA_ATTENTION_TYPE_CAUSAL /
   LLAMA_ATTENTION_TYPE_NON_CAUSAL on params_base.attention_type.
   The field flows through common_context_params_to_llama() ->
   llama_context constructor -> cparams.causal_attn.

2. Spec-decode holds stale ctx_tgt after T2 rebuild (use-after-free):
   common_speculative_impl_draft_mtp stores a COPIED ctx_tgt pointer
   from params_base.speculative.draft.ctx_tgt at init time. After
   T2 rebuilds the context, this pointer is dangling. Fix: update
   params_base.speculative.draft.ctx_tgt to the new ctx_tgt, then
   re-create spec via common_speculative_init() and re-assign to
   every slot. Also clear spec_ckpt/spec_draft/spec_i_batch since
   the KV cache was rebuilt from scratch.

3. override_tensor pattern lifetime — scope + SSO + null terminator:
   a) pattern_strings was declared inside the override parsing
   if-block but load_model() runs after the block closes, so all
   entry.pattern C strings were dangling. Move declaration to
   function scope alongside swapped_params.
   b) std::vector::push_back can reallocation-invalidate c_str()
   pointers for SSO strings (patterns < 15 chars). Add reserve(16)
   to prevent reallocation.
   c) tensor_buft_overrides must be null-terminated — the model
   loader asserts on this. Add {nullptr, nullptr} sentinel.

4. Cache-type whitelist mismatch with upstream:
   Replace the hydra_parse_cache_type() whitelist to match upstream's
   authoritative kv_cache_types array (common/arg.cpp:390): add
   GGML_TYPE_BF16 and GGML_TYPE_IQ4_NL, remove GGML_TYPE_Q8_1.

Refs: #41 (follow-up review), #42 (follow-up review)

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

The previous commit added spec re-init on apply_t2_rebuild's success
path, but the rollback path (first context-recreate fails, falls back
to old params) also creates a fresh ctx_tgt without re-initing spec.
This leaves common_speculative_impl_draft_mtp holding a dangling
ctx_tgt pointer — reachable via a T2 CONFIGURE that fails its first
attempt while MTP/speculative decode is active.

Mirror the same if (spec) { ... } re-init block into the rollback
branch.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signals (SIGCHLD from subprocess exit, etc.) can cause recv() to
return EAGAIN/EWOULDBLOCK even on blocking sockets with SO_RCVTIMEO.
The old code treated every recv failure as fatal, closing the RPC
connection and surfacing as engine_rpc_error prefill fallbacks.

hydra_recv_all now retries on EINTR (transient signal) and retries
once on EAGAIN/EWOULDBLOCK to distinguish signal-induced transients
from a genuine SO_RCVTIMEO idle timeout (120s). A second consecutive
EAGAIN is a real timeout — connection dropped as intended.

Fixes the intermittent 21% COMBINED prefill fallback rate on DENSE 27B.

ddvnguyen/hydra_vortex#382

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
set_hydra_capabilities was hardcoded to split_mode='solo' and
set_hydra_combined_static was never called, so tensor-split engines
(DENSE profile) always reported mode='solo' to the Hydra control
plane. This caused SetExpertMode('combined') to fall through to
the expert-split path which requires --combined-ot-pattern (absent
for DENSE), resulting in 'staying solo' fallback.

Fix: detect tensor_split at init, pass split_mode='layer' and call
set_hydra_combined_static(true). The layer-split SetExpertMode
path (line 3906) then correctly handles 'combined' as a no-op success.

ddvnguyen/hydra_vortex#382

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

The engine owns the drain decision, not Hydra Core. Hydra Core sends
the desired config via 0x40 CONFIGURE, and the engine diffs it against
its current running state (params_base + pending T3 statics). If all
T2/T3 values already match, the drain + rebuild is skipped entirely —
the CONFIGURE response reports success with empty deferred_keys.

Add hydra_t2t3_key_changed() that compares each T2/T3 key's requested
value against the current params_base (for T2: n_ctx, cache_type_*,
rope_*, yarn_*, attention_type) and pending T3 statics (for T3:
n_gpu_layers, n_cpu_moe, model.path, override_tensor). Keys that
match are filtered out before staging. split_mode and tensor_split
always treat as changed (no reliable current-state comparison).

Refs: #42

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

load_model(swapped_params) copies params into params_base (line 844),
which includes tensor_buft_overrides entries whose entry.pattern is a
raw const char* into pattern_strings. If pattern_strings is a local
variable in apply_t3_rebuild(), it is destroyed at function exit,
leaving dangling pointers in params_base.tensor_buft_overrides.

Move pattern_strings to server_context_impl::hydra_pattern_strings
(member scope) so it outlives apply_t3_rebuild(). Cleared explicitly
on each T3 apply.

Refs: #42 (follow-up review P0)

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

P1: T3 teardown breaks layer-split COMBINED reloads. For
hydra_combined_static (layer-split, DENSE profile), the teardown
removes the peer from the ggml backend registry. Then load_model()
builds the new model without the peer, placing the entire model on
the local GPU — OOM on next prefill. The reattach only sets a flag
without re-registering the peer.

Fix: gate the teardown on hydra_combined_static. For layer-split,
only zero the mode flag — leave the peer registered so load_model()
can see it during tensor_split placement. For expert-split, the full
teardown is required.

P2: Consolidate hydra_current_peer vs hydra_peer in teardown. Use
hydra_current_peer consistently for both remove and clear — it's the
endpoint actually registered in the ggml backend registry (SET_EXPERT_MODE
registers via hydra_current_peer). The reattach paths correctly use
hydra_peer for re-registration.

P2: Extract refresh_slots_after_ctx_rebuild() helper. Both the success
and rollback paths of apply_t2_rebuild had byte-for-byte identical
15-line blocks (slot pointers + smpl + spec reinit). This was the
exact bug class where the rollback path originally missed the spec
reinit. Single helper eliminates the possibility of recurrence.

Refs: #42 (follow-up review)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
P3 thread-model migration. The six s_hydra_pending_* statics
(override_tensor, split_mode, tensor_split, n_gpu_layers,
n_cpu_moe, model_path) in llama-hydra.cpp are moved to
llama_context fields (hydra_pending_*). All getters/setters
now take a llama_context* parameter and read/write the
context's own fields instead of file-scope statics.

This eliminates the hard threading invariant documented in
the old code: 'CONFIGURE handler and update_slots must run on
the same thread.' Each context now owns its own pending T3
state, so concurrent contexts (future multi-model or
multi-worker) won't collide.

P3b (async T3 reload) is deferred to a follow-up — load_model()
replaces nearly every member variable, making background-thread
reload require double-buffering, which is a larger architectural
change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
n_cpu_moe: always return true (no params equivalent; informational
only — apply path just logs it).

model.path: fall back to params.model.path when pending static is
empty, so re-sending the current path doesn't trigger a spurious
T3 reload (same pattern as n_gpu_layers).

override_tensor: always return true (no params equivalent; apply
path parses and applies on model reload).

Refs: #42

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ddvnguyen ddvnguyen force-pushed the fork/phase-2b-t2t3-apply branch from 5ab801c to 26d2d53 Compare July 12, 2026 03:50
@ddvnguyen ddvnguyen merged commit 19b2d38 into fork/phase-2b-configure-extension Jul 12, 2026
ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jul 12, 2026
…dule to PR #42

Fork PR #42 (ddvnguyen/llama.cpp#42) implements the T2/T3 apply
path (apply_t2_rebuild + apply_t3_rebuild in server-context.cpp).
This actually reloads llama_context (T2) and llama_model (T3)
when the slot-free moment fires, completing the work that PR #41
started.

This commit:

1. Bumps the submodule pointer from 6830c25 (PR #41 head) to
   dd521b1 (PR #42 head). Pre-PR reachability check passes.

2. Updates docs/system-test-phase-2b.md to:
   - Mark T1/T2/T3 all as end-to-end PASS in the tier matrix
   - T2 test: update the trigger path (EngineConfigApplier call),
     the expected log lines (now reflects the real T2 rebuild), and
     add the T2 failure recovery section (rollback + GGML_ABORT
     catastrophic case)
   - T3 test: update the trigger path, the expected log lines
     (now includes the COMBINED teardown + reattach logs), and
     add the T3 failure recovery section
   - Pass/fail criteria: tighten to confirm all three tiers work
     end-to-end
   - Cross-references: add #42 alongside #41

The doc now reflects the post-PR-#42 state. T1 was always
end-to-end; T2 and T3 are now end-to-end. The unit tests
(test-hydra-configure-tier, test-hydra-state-chunk-size,
test-hydra-checkpoint-policy, test-hydra-rpc-bind) all pass
against the new binary; both sm_120 and sm_60 builds are clean.

Refs: ddvnguyen/llama.cpp#42, #406,
#407

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jul 12, 2026
)

Updates the submodule pointer from dd521b10 to cac7122a3, which
includes the review findings addressed on ddvnguyen/llama.cpp#41
and ddvnguyen/llama.cpp#42:

- PR #42 P1: params_base confirmed updated by load_model() (comment)
- PR #42 P1: SRV_ERR + SRV_INF log around T2 rollback + GGML_ABORT
- PR #42 P2: override_tensor pointer lifetime documented
- PR #42 P2: hydra_peer vs hydra_current_peer verified correct
- PR #41 P1: T3 statics thread model invariant documented
- PR #41 P1: n_parallel=-1 fix (UINT32_MAX wrap to 1)

Pre-PR reachability: cac7122a3 is on
https://github.com/ddvnguyen/llama.cpp refs/heads/fork/phase-2b-t2t3-apply

Refs: ddvnguyen/llama.cpp#41, ddvnguyen/llama.cpp#42,
ddvnguyen/llama.cpp#43 (RPC response delivery bug — blocks E2E)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

1 participant