Skip to content

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

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

fix(server): validate sampling params and rebuild timeout at the API layer#223
Artemowka22 wants to merge 1 commit 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.
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