[review-finding] llama#70: fit_params skip in apply_t3_rebuild() isn't gated on is_first_load, contrary to its own justification

[ddvnguyen]

From review of PR #70 (fix/507-t3-fit-params-skip, commit 07cd903).

Finding

apply_t3_rebuild() (tools/server/server-context.cpp:5124) now unconditionally sets swapped_params.fit_params = false; before every load_model() call in this function, justified by the comment:

(a) we just freed VRAM by destroying the old model, (b) the new model's requirements are known (same or smaller), (c) a controlled inference server has predictable VRAM.

apply_t3_rebuild() is not only the hot-swap/reload path — it is also the code path for the engine's true first model load (the "modelless boot, wait for hydra_config" pattern this fork uses for model-agnostic workers). Confirmed by tracing both call sites (server-context.cpp:2610, :4657) and the first_load_pending staging path used when ctx_tgt is still null. On that path, is_first_load == true and justification (a) does not hold — no VRAM was freed, because no model was ever loaded.

The swapped_params.fit_params = false assignment is not gated on is_first_load (the existing bool is_first_load = !ctx_tgt; at the top of the function), even though the very next block (if (!load_model(...))) already branches on it.

Impact

Today this is a no-op: infra/hydra-core/config/models.json's engine_defaults.fit = false globally, and no model overrides it to true, so params_base.fit_params is already false for every currently-configured model by the time apply_t3_rebuild() copies it into swapped_params.

The risk is latent: any future model added without hand-tuned n_gpu_layers/tensor_split — relying on fit=true for VRAM-based auto-sizing on first load, which is fit_params's documented purpose (common/common.h:453) — would have that request silently overridden to false, turning what should be a graceful auto-downsize into a hard load failure ("hydra: T3 first load failed — engine stays empty"). There is no log line distinguishing "fit was requested and skipped by this override" from "fit wasn't requested at all," which would make this confusing to diagnose if it recurs.

Suggested fix

Gate the skip so it only applies when there's actually a prior model to have freed VRAM from, e.g.:

if (!is_first_load) {
    swapped_params.fit_params = false;
}

or scope it further to only the was_combined / confirmed-reload case, matching the surrounding COMBINED-teardown gating a few lines above.

Severity

Non-blocking — matches nothing in the currently-deployed model fleet — but a one-line gate now avoids a confusing failure mode later. Flagged in review of #70 rather than blocking, since #70 is an explicit partial fix (PR body: "the remaining ~3+ minutes are not yet explained") paired with hydra_vortex#510's coordinator-side timeout headroom as the actual safety net.

Ref: PR #70, review comment.