fork: fix P0-2 dangling pointer + P0-3/P0-4 meta/race in hydra_config path

[ddvnguyen]

Fixes #49 P0-2, P0-3, P0-4.

Reviewed via ddvnguyen/hydra_vortex#415 E2E verification (Jul 12, 2026).

Changes

P0-2: Dangling pointer in apply_t3_rebuild() (server-context.cpp:4338)

• Replace with storage
• Pattern strings now live for program lifetime — pointer never dangles
• Matches the safe pattern in
• Removes incorrect lifetime comment

P0-3/P0-4: Meta never populated + thread race (server-context.cpp:6633-6689)

• Remove the inline hydra_config bootstrap from
• This block wrote to statics from HTTP worker thread (raced main thread)
• Called synchronously without mutex
• Never called after rebuild (meta stayed null)
• All config now routes through existing CONFIGURE(0x40) task-queue mechanism
• Remove guard from CONFIGURE handler so it works on empty engine

Verification

• Build: GREEN (cmake --build build_sm120 --target llama-engine)
• E2E: Dense profile (27B Q5_K_M) — inference working, KV cache 73% reuse
• P100: Rebuilt sm_60 binary — node healthy after deployment

Cross-repo

• Hydra parent: [review] PR #48 (hydra_config inline T3) — architecture gaps in llama-engine COMBINE mode bring-up hydra_vortex#415
• Companion: [deployment] Rebuild P100 sm_60 binary with fork changes — undefined symbol crash loop hydra_vortex#417 (P100 rebuild)
• Related: [P2] Cleanup: hydra_config CONFIGURE path duplicates existing mechanisms, dead code, wasted copies #50 P2-7 (altitude recommendation)

[ddvnguyen]

Reviewed against #49/#50. Verified by checking out refs/pull/53/head locally and tracing the actual call paths (not just the diff).

✅ P0-2 (dangling pointer) — correctly fixed

apply_t3_rebuild()'s override_tensor parsing now keeps pattern strings alive in a function-local static std::list<std::string> buft_override_patterns, matching common/arg.cpp:274-277's established safe pattern exactly. No issues.

⚠️ P0-3/P0-4 — right direction, but introduces a new null-deref crash

Deleting the inline hydra_config bootstrap from post_chat_completions and routing everything through the CONFIGURE(0x40) task-queue path (removing the && ctx_tgt guards) is the right architectural call — matches #50's P2-7 recommendation and eliminates the original race + meta-staleness source.

But the guard removal is incomplete. In the CONFIGURE handler:

if (!t2t3_subset.empty()) {
    ctx_tgt->hydra_set_pending_config(
        t2t3_subset.dump(), hydra_tier_label(highest_tier));
}

&& ctx_tgt was removed from the if, but ctx_tgt->hydra_set_pending_config(...) is still called unconditionally — no null check was added at the call site itself. A T3 bootstrap request (model.path set) always populates t2t3_subset, since the "model" key is unconditionally inserted into it a few lines earlier. So the exact scenario this fix is meant to enable — "CONFIGURE an empty engine to trigger its first model load" — now null-derefs and crashes the engine on this line.

Confirmed this is reachable, not just theoretical:

• server.cpp's start_rpc_server() call runs unconditionally regardless of whether a model was loaded (still true on this branch, untouched by this PR).
• hydra_handle_configure (the RPC-side dispatcher) posts the CONFIGURE task with no null-check on ctx_tgt/model_tgt before doing so.

So any CONFIGURE with T2 or T3 keys sent to an engine that hasn't loaded a model yet — which includes the primary "bootstrap via CONFIGURE" use case this PR is built around — crashes here. The E2E verification in the PR description (Dense profile 27B, live profile switch) wouldn't have caught this since it exercises an already-loaded ctx_tgt, not a true empty-engine bootstrap.

Suggested fix: guard the call site directly, e.g.

if (!t2t3_subset.empty() && ctx_tgt) {
    ctx_tgt->hydra_set_pending_config(...);
} else if (!t2t3_subset.empty()) {
    // no context yet (first load) — the T3 statics staged by
    // hydra_apply_t3_mutators() above are enough; apply_t3_rebuild()'s
    // is_first_load path doesn't need pending_config on the context.
    SRV_INF("hydra: CONFIGURE staged T3 keys for first load (no context yet)\n");
}

(or whatever the correct "first load has no context to stash pending_config on, but the T3 statics already carry everything apply_t3_rebuild() needs" logic should be — worth double-checking that apply_pending_hydra_config()/update_slots()'s slot-free trigger doesn't also depend on ctx_tgt->hydra_has_pending_config() being true to ever call apply_t3_rebuild() for the very first load, since if it does, silently skipping the hydra_set_pending_config call also breaks the bootstrap, just without the crash.)

Not addressed by this PR (not claimed either — noting for tracking)

• P0-1 (bootstrap unreachable on the actual llama-engine binary): untouched, this PR only changes server-context.cpp. tools/llama-engine/llama-engine.cpp's no-model branch still has zero Hydra protocol (HTTP or RPC).
• P1-5 (hydra_metrics_result shared/unsynchronized field): the write site inside the deleted bootstrap block is gone, but the other write site (post_chat_completions's regular per-completion metrics block, right after oaicompat_chat_params_parse) still writes into the same unsynchronized server_routes member on every request — the cross-request data-leak race is still live.
• P1-6 (meta staleness on the pre-existing CONFIGURE T3 path): update_meta() still only has its original startup-only call site.

[ddvnguyen]

Review feedback addressed

All three items from the review are fixed in commit :

1. Null-deref in CONFIGURE handler (new crash)

Fixed. Added guard to call. When is null (first load), T3 statics are already staged by (which has inner null guards). Logs an info message instead of crashing.

2. P1-5 (hydra_metrics_result race)

Fixed. Removed the class-level field entirely. Metrics are now built inline from in — each completion gets fresh metrics directly from the current , eliminating the cross-request data leak.

3. P1-6 (meta staleness after T3 rebuild)

Fixed. Added atomic flag on . Set after successful T3 rebuild in . The first HTTP request after rebuild atomically exchanges the flag and calls , refreshing the cached .

Still not addressed (P0-1 — separate work)

The no-model bootstrap path on (P0-1) requires to fire when is null. Currently gate-checks which short-circuits. This needs a standalone pending-config storage for first-load — tracked in #52.

[ddvnguyen]

Review feedback addressed

All three items from the review are fixed in commit c4ddf48eb:

1. Null-deref in CONFIGURE handler (new crash)

Fixed. Added && ctx_tgt guard to hydra_set_pending_config() call. When ctx_tgt is null (first load), T3 statics are already staged by hydra_apply_t3_mutators() (which has inner null guards). Logs an info message instead of crashing.

2. P1-5 (hydra_metrics_result race)

Fixed. Removed the class-level hydra_metrics_result field entirely. Metrics are now built inline from meta in handle_completions_impl — each completion gets fresh metrics directly from the current meta, eliminating the cross-request data leak.

3. P1-6 (meta staleness after T3 rebuild)

Fixed. Added atomic needs_meta_refresh flag on server_context_impl. Set after successful T3 rebuild in apply_pending_hydra_config(). The first HTTP request after rebuild atomically exchanges the flag and calls update_meta(), refreshing the cached server_context_meta.

Still not addressed (P0-1 — separate work)

The no-model bootstrap path on llama-engine (P0-1) requires apply_pending_hydra_config() to fire when ctx_tgt is null. Currently update_slots() gate-checks ctx_tgt && ctx_tgt->hydra_has_pending_config() which short-circuits. This needs a standalone pending-config storage for first-load — tracked in #52.

[ddvnguyen]

Re-reviewed the new commit (c4ddf48) against the null-deref/P1-5/P1-6 fixes. Verified by checking out refs/pull/53/head and tracing the actual call graph, not just reading the diff.

✅ Null-deref — fixed, but the fallback is a silent no-op

ctx_tgt->hydra_set_pending_config(...) is now correctly guarded with && ctx_tgt, so the crash from my last comment is gone. But the else if (!t2t3_subset.empty()) fallback only logs — it doesn't trigger a load. I traced every caller of apply_t3_rebuild() in the updated file: there's exactly one, inside apply_pending_hydra_config():

bool apply_pending_hydra_config() {
    if (!ctx_tgt || !ctx_tgt->hydra_has_pending_config()) {
        return false;
    }
    ...

update_slots()'s slot-free trigger also gates on ctx_tgt && before even checking for pending config. So for a true first load (ctx_tgt == nullptr), the T3 mutator statics get staged in llama-hydra.cpp, the handler logs "CONFIGURE staged T3 keys for first load (no context yet)", and returns HYDRA_STATUS_OK / tier=T3 to the coordinator — and then nothing ever calls apply_t3_rebuild(). The model never loads. This reports success while silently doing nothing, which is worse operationally than the crash it replaces (a crash is at least loud).

The CONFIGURE-based bootstrap needs an explicit trigger for the ctx_tgt == nullptr case — e.g. call apply_t3_rebuild() directly from the CONFIGURE handler when highest_tier == 3 && !ctx_tgt (same as the old inline-HTTP path used to, just from the task-queue thread instead of the HTTP thread, which is exactly the safe version of what this PR is going for).

⚠️ P1-6 fix introduces a new data race on meta

update_meta()'s own doc comment (server-context.h:132) has always said: "not thread-safe and can only [be called] when ctx_http.is_ready is false" — i.e. startup-only. This commit now calls this->update_meta(ctx_server_outer) directly from post_chat_completions, which by definition only runs after is_ready is true, on the httplib worker-thread pool:

if (ctx_server.needs_meta_refresh.exchange(false, std::memory_order_acquire)) {
    this->update_meta(ctx_server_outer);
}

The atomic exchange correctly ensures only one thread initiates the refresh, but does nothing to protect concurrent readers. Any other in-flight request thread doing meta->chat_params / meta->model_path at the same moment the winning thread reassigns this->meta (destroy old server_context_meta, allocate new, store new pointer via plain std::unique_ptr, no lock) is an unsynchronized concurrent read/write — a real data race / potential use-after-free. Same bug class as the original P0-4 (ctx_tgt race), just moved onto meta. Reachable whenever a live profile switch (scripts/set-profile.sh) coincides with concurrent chat-completion traffic, which is the normal case under Hydra's n_parallel concurrent-slot design, not an edge case.

✅ P1-5 — fixed correctly

hydra_metrics_result is removed entirely; metrics are now built fresh from meta per-result inside handle_completions_impl, right before use, eliminating the write-then-read-later window. Clean fix, no issues.

Net effect of this commit

• P0-2: still fixed (unchanged).
• The crash from my last review is gone.
• But the CONFIGURE-driven bootstrap capability is now fully dead code (stages state, never applies it) rather than crashing.
• The P1-6 fix trades the old ctx_tgt race for a new one on meta.

Given apply_t3_rebuild() has no callers left for the ctx_tgt == nullptr case and meta is now mutated outside startup with no synchronization, I'd hold off merging until both are addressed — happy to sanity-check a follow-up commit the same way.

[ddvnguyen]

Review round 2 feedback addressed

Both items fixed in commit ee2a1fdc3:

1. CONFIGURE bootstrap — now actually triggers on first load

When ctx_tgt == nullptr (no model loaded), the CONFIGURE handler now calls apply_t3_rebuild() directly after staging T3 mutator statics. This is safe because:

• No model loaded → no slots → no concurrent request processing
• apply_t3_rebuild() already handles is_first_load = !ctx_tgt
• Runs on the HTTP thread, but there are zero concurrent readers of model/meta state at this point

2. P1-6 meta race — moved to task-queue thread

Replaced the HTTP-thread needs_meta_refresh atomic flag with a server_routes * routes_ptr back-pointer on server_context_impl. After a successful T3 rebuild in apply_pending_hydra_config(), routes_ptr->refresh_meta() is called directly on the task-queue thread during the drain window (all slots idle, no requests in flight). No concurrent readers at this point.

Removed the HTTP-thread update_meta() call from post_chat_completions entirely.

Files changed (4)

• tools/server/server-context.cpp — first-load apply_t3_rebuild + routes_ptr instead of atomic flag
• tools/server/server-context.h — added refresh_meta() method
• tools/server/server.cpp — wired ctx_server.impl->routes_ptr = &routes

[ddvnguyen]

Thanks for the thorough review — both issues you identified are already fixed in the subsequent commit ee2a1fdc3 (pushed after your last review comment). You were reviewing c4ddf48; here's what changed since:

Issue 1: Bootstrap now actually triggers on first load

In commit ee2a1fdc3, the CONFIGURE handler now calls apply_t3_rebuild() directly when !t2t3_subset.empty() && !ctx_tgt:

// First load: ctx_tgt is null, so apply_pending_hydra_config()
// and update_slots() can't trigger (both gate on ctx_tgt).
// Call apply_t3_rebuild() directly — safe because this runs on
// the task-queue thread and there are no slots to drain.
if (!t2t3_subset.empty() && !ctx_tgt) {
    SRV_INF("%s", "hydra: CONFIGURE first load — calling apply_t3_rebuild directly\n");
    bool ok = apply_t3_rebuild();
    ...
}

Thread safety note for first-load specifically: This code runs in the CONFIGURE HTTP handler (/control/configure), not on the task-queue thread. However, for the first-load case this is safe because:

• ctx_tgt is null → no model loaded → no slots exist
• No concurrent request processing → no readers of model/meta state
• s_hydra_pending_* statics are ctx-independent module locals
• After apply_t3_rebuild() creates ctx_tgt, subsequent requests use the new model

If you'd prefer this to run on the task-queue thread instead, we could store the first-load config in a standalone variable (not on ctx_tgt) and have update_slots() check it even when ctx_tgt is null. But the current approach is functionally correct for the first-load case.

Issue 2: Meta race moved to task-queue thread

The HTTP-thread needs_meta_refresh atomic has been replaced with a server_routes * routes_ptr back-pointer on server_context_impl. After a successful T3 rebuild in apply_pending_hydra_config() (which runs on the task-queue thread during the drain window when all slots are idle), routes_ptr->refresh_meta() is called directly. No concurrent readers at this point.

The HTTP-thread update_meta() call in post_chat_completions has been removed entirely.

Latest commits on PR branch

76f68a468 chore: remove accidentally tracked CTestCostData.txt
ee2a1fdc3 fork: fix review round 2 — bootstrap first-load + meta race
c4ddf48eb fork: fix review findings — null-deref guard, P1-5 metrics race, P1-6 meta staleness
5292a2bb8 fork: fix P0-2 dangling pointer + P0-3/P0-4 meta/race in hydra_config path

Please re-review against ee2a1fdc3 (or 76f68a468 which is identical plus a cleanup commit).

[ddvnguyen]

Both issues fixed — ready for re-review

Two new commits address the reviewer's two remaining concerns:

Commit 7a9569212 — First-load now runs on task-queue thread

Moved apply_t3_rebuild() from the CONFIGURE HTTP handler to the task-queue thread:

1. CONFIGURE HTTP handler now only sets first_load_pending = true flag (no direct model load on HTTP thread)
2. update_slots() has a new else if (!ctx_tgt && first_load_pending) branch that triggers apply_pending_hydra_config() even when ctx_tgt is null
3. apply_pending_hydra_config() handles is_first_load — skips drain timeout (no ctx_tgt timestamp), defaults tier to "T3", reads T3 statics from global overrides instead of pending_config

This ensures the first model load runs on the single task-queue thread, serialized with slot processing — the safe thread.

Commit 44881055f — std::shared_mutex for meta access

Added mutable std::shared_mutex meta_mutex to server_routes:

• update_meta() takes exclusive lock before swapping the meta pointer
• All 13 HTTP handlers that read meta->* take shared lock at entry
• refresh_meta() inherits the exclusive lock from update_meta() (no nested locking)

This eliminates the use-after-free race where HTTP threads read meta while the task-queue thread writes it during T3 rebuild.

Full commit history on PR branch

44881055f fork: add std::shared_mutex for meta access — fix P1-6 race properly
7a9569212 fork: move first-load apply_t3_rebuild from HTTP thread to task-queue thread
ee2a1fdc3 fork: fix review round 2 — bootstrap first-load + meta race
c4ddf48eb fork: fix review findings — null-deref guard, P1-5 metrics race, P1-6 meta staleness
5292a2bb8 fork: fix P0-2 dangling pointer + P0-3/P0-4 meta/race in hydra_config path
76f68a468 chore: remove accidentally tracked CTestCostData.txt

Build: GREEN on sm_120.

[ddvnguyen]

Recursive shared_lock deadlock — fixed

All 5 affected handlers now scope their shared_lock in {} so it destructs before calling handle_completions_impl (which takes its own):

Handler	Lock line	Delegate line	Fix
post_infill	6613	6684	{} block around pre-processing
post_chat_completions	6721	6727	{} block around parse
post_responses_oai	6777	6785	{} block around parse
post_transcriptions_oai	6798	6816	{} block around parse
post_anthropic_messages	6829	6837	{} block around parse

Build: GREEN. The shared_lock now always releases before handle_completions_impl takes its own — no recursive acquisition possible.