Skip to content

fix(server): validate sampling params and rebuild timeout at the API layer - #223

Open
Artemowka22 wants to merge 2 commits into
FlashML-org:mainfrom
Artemowka22:fix/api-input-validation
Open

Artemowka22 wants to merge 2 commits into
FlashML-org:mainfrom
Artemowka22:fix/api-input-validation

Conversation

@Artemowka22

Copy link
Copy Markdown

Summary

  1. Invalid sampling silently reaches the engine. resolve_sampling forwards arbitrary client floats: temperature=-3, NaN, inf, top_p=0, top_p=7, top_k=0 all become a SamplingParams and die (or misbehave) deep in the sampler instead of failing the request. Fix: after checkpoint defaults are filled, validate temperature (finite, ≥ 0), top_p ((0, 1]), top_k (−1 or ≥ 1) and raise ValueError in the same style as the existing max_tokens check — every adapter already maps that to a clean HTTP 400.

  2. CacheRebuildRequest.timeout is unbounded. A negative/NaN/huge timeout reaches asyncio.wait_for on the rebuild future; inf parks the handler forever. Fix: pydantic bound 0 < timeout ≤ 3600.

The audit that surfaced these (agisota/freetoken-mlx, docs/AUDIT.md, defects 3–4) also reported a TOCTOU race on concurrent /v1/cache/rebuild; current upstream has already restructured that path (single synchronous section from the pre-flight checks to maintenance_state = "rebuilding"), so only the timeout bound is ported.

Test plan

  • parametrized invalid/boundary sampling matrix on /v1/chat/completions in tests/server/test_openai_api.py — 400 with the offending parameter named; boundary values temperature=0, top_p=1, top_k=-1, top_k=1 accepted
  • rebuild timeout bounds in tests/server/test_rebuild_maintenance.py
  • full tests/server/ suite passes

resolve_sampling forwarded arbitrary client floats into SamplingParams: negative,
NaN and inf temperature, top_p outside (0, 1], top_k 0 -- all reached the engine
and died (or misbehaved) deep in the sampler instead of failing the request.
Validate the RESOLVED values (checkpoint defaults included) and raise ValueError
in the style of the existing max_tokens check; every adapter already maps that to
a clean 400. CacheRebuildRequest.timeout gets the matching pydantic bound
(0 < t <= 3600) so a NaN/inf/negative timeout cannot reach asyncio.wait_for.

Found via the freetoken-mlx downstream audit (docs/AUDIT.md, defects 3-4); the
rebuild TOCTOU race from that audit is already structurally fixed upstream, so
only the timeout bound is ported.
@gdevenyi

gdevenyi commented Sep 5, 2026

Copy link
Copy Markdown

Tried on 2 x RTX 6000 Ada (sm_89) serving Qwen3.8-Flash-Next (RadixArk NVFP4) at TP=2, offload backend, fp8 KV pool of 8 x 262,144 tokens, merged onto my deploy branch (main af71ba4 + #385/#386/#389/#392/#354 and ten other open PRs), tests run on the box, then put in production.

Probes against the live server: temperature=-1, top_p=0, top_k=0 each answer 400 with the message from this PR (temperature must be a finite number >= 0, got -1.0 and so on). tests/server passes. Merged clean, in production.

@Artemowka22

Copy link
Copy Markdown
Author

Thank you for the production validation — live probes against a deployed server (and the exact 400 messages coming back as written) is better coverage than anything we could add here. Nothing to change on our side; noting for the record that this PR has now been run in production on sm_89 with TP=2 and the offload backend.

One conflict, in python/freetoken/server/generation.py: upstream made the
default output budget configurable (default_max_tokens, FlashML-org#411) in the same
SamplingParams construction this branch rewrote to return the validated
values. Kept the validated resolved_* fields; the max_tokens fallback now
uses the configurable default_max_tokens instead of the fixed constant.

Assisted-by: Claude
MT-z added a commit to MT-z/FreeToken that referenced this pull request Sep 16, 2026
's temperature/top_p/top_k

Assisted-by: Claude Opus 5
MT-z added a commit to MT-z/FreeToken that referenced this pull request Sep 16, 2026
…oudly, measured on this box)

Artemowka22's FlashML-org#223, open since 2026-08-26. `resolve_sampling` forwarded arbitrary client
floats; the PR validates `temperature`, `top_p` and `top_k` after the checkpoint defaults
are filled, and bounds `CacheRebuildRequest.timeout` to (0, 3600].

gdevenyi probed the live server on his deploy branch and reported the 400s. What this box
adds is the before: the PR says invalid values "die (or misbehave) deep in the sampler",
and neither is what happens here.

    try/all b3e2b21, every case answered 200 and the server stayed up:
      temperature=-3, top_p=0, top_p=7, top_k=0, top_k=-5   200, ordinary-looking text
      temperature=NaN, temperature=inf                      200, output was '!!!'

Nothing dies. The two failure shapes are a client getting a normal-looking answer that was
not sampled the way it asked, and a client getting '!!!' -- both with no way to tell. That
is worse than the missing 400, and it is the same family as the rest of FINDINGS.

With the merge, all seven answer 400 naming the value (`temperature must be a finite
number >= 0, got nan`, `top_p must be in (0, 1], got 7.0`, `top_k must be -1 (disabled) or
>= 1, got 0`), and the four boundaries -- temperature=0, top_p=1, top_k=-1, top_k=1 --
still answer 200.

Conflicted with FlashML-org#393 in `generation.py`, and the resolution is the point: FlashML-org#393 already
validates min_p, presence/frequency_penalty, repetition_penalty, min_tokens, logit_bias
and stop_token_ids -- the parameters it added -- and FlashML-org#223 covers the three core knobs it
did not touch. Complementary, not competing, so both sets are kept and the constructor
takes FlashML-org#223's resolved values. Verified after the merge that FlashML-org#393's side still fires: all
seven of its checks answer 400 with their own messages.

Also committed conflict markers once by running `git add -A` on a tree where `tail -2`
had hidden the second CONFLICT line; the server then failed to start with a SyntaxError
and the whole "after" probe was void. Re-measured.

Full suite: 11 failed, 1858 passed, 60 skipped -- the same 11 (5 upstream's own, 6 from
FlashML-org#354), plus the 6 tests the PR adds.

Assisted-by: Claude Opus 5
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.

2 participants