Skip to content

fix: normalise logits_processors row slots dropped to None by batch merge - #1799

Merged
jundot merged 2 commits into
jundot:mainfrom
efortin:fix/logits-processors-extend-none-slots
Jun 11, 2026
Merged

fix: normalise logits_processors row slots dropped to None by batch merge#1799
jundot merged 2 commits into
jundot:mainfrom
efortin:fix/logits-processors-extend-none-slots

Conversation

@efortin

@efortin efortin commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

While running mixed workloads against a single model — a regular chat completion decoding next to a structured response_format: json_schema request — the server reliably falls into the corruption-recovery loop and ends up killing every in-flight request with Cache corruption not recoverable after retries: 'NoneType' object is not iterable. Single requests are fine, clearing caches changes nothing, and it reproduces on the first mixed wave every time. Full analysis and a standalone repro script are in #1798.

It turns out this is not cache state at all. #1747 made sure we always hand logits_processors to BatchGenerator.insert() as per-row lists ([[]], never None), which fixed the single-request crash from #934. But when the scheduler merges two running batches, mlx-lm's GenerationBatch.extend() rebuilds the row slots on its own:

# mlx_lm/generate.py
if not any(self.logits_processors):
    self.logits_processors = [None] * len(self.uids)

any([[], []]) is False, so the empty-list shape we pass at insert time collapses right back to None slots whenever a batch with no active processor merges with a grammar-constrained one. The next _step then iterates self.logits_processors[e] over a None slot and raises. Recovery clears the caches, re-prefills, re-merges the same rows… and crashes again, every ~1.5 s, until the retry budget gives up and fails everyone. That is why it presents as unrecoverable cache corruption when it is really a batch-merge shape bug.

Fix

Normalise the row slots — not just the whole list — in _patched_generation_batch_step, right before the original step runs. That is the one chokepoint both origins (insert and merge) funnel through, and it also protects our own grammar-accept loop right below, which iterates the same slots. The insert-side wrapping from #1747 stays as-is.

Tests

  • unit test of the chokepoint normalisation (monkeypatched original step, no model needed)
  • source-level assertion so the per-row normalisation cannot be silently dropped later
  • integration test reproducing the actual gap: a grammar row decoding, then a plain row carrying the [[]] "fix shape" joining through extend() — crashes without this patch, passes with it (gated behind VLLM_MLX_INTEGRATION=1 like the existing qwen3.6-35b-a3b and qwen3.6-27b infinite loop #934 tests)
  • the module docstring of the regression tests now documents the merge origin

Verification

Before: a first wave of 3 streaming chats (thinking on) + 3 structured requests joining mid-decode deterministically killed 8/8 in-flight requests on the 0.4.3 bundle (6/8 on source @ e892ef4) in ~8 s, with the recovery loop visible in the log.

After: same harness, 4 rounds / 32 mixed concurrent requests — zero corruption markers in the debug log, zero request errors, every stream completes normally.

pytest tests/test_scheduler_logits_processors.py -v -m "not integration" → 6 passed.

Fixes #1798

…erge

GenerationBatch.extend() re-creates None row slots on heterogeneous
continuous-batch merges (`any([[], []])` is False), undoing the
insert-side empty-list shape from jundot#1747 whenever a request without
active processors joins a batch serving a grammar-constrained one.
The next _step then crashes with "'NoneType' object is not iterable",
and the corruption recovery re-merges the same rows, looping until the
retry budget fails every in-flight request.

Normalise every per-row slot at the _patched_generation_batch_step
chokepoint, covering both the insert and merge origins, and pin the
invariant with unit, source-level, and integration regression tests.

Fixes jundot#1798
@efortin
efortin force-pushed the fix/logits-processors-extend-none-slots branch from 127f546 to 8c0b900 Compare June 10, 2026 16:18
@jundot

jundot commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Thanks for tracking this down. I verified the root cause against the pinned mlx-lm path: empty per-row processor lists can be turned back into None during prompt-batch merging and then reach GenerationBatch._step, so normalizing per-row slots at the _step chokepoint is the right fix. The targeted non-integration tests pass locally. This looks good to me, and I am going to merge it.

@jundot
jundot merged commit 33e57aa into jundot:main Jun 11, 2026
@efortin
efortin deleted the fix/logits-processors-extend-none-slots branch June 11, 2026 05:47
efortin added a commit to efortin/omlx that referenced this pull request Jun 11, 2026
jundot#1799 made the generation step crash-safe by normalising None row slots,
but the positional drift behind it was still there: a stale or offset
slot left in samplers/logits_processors by batch extend/filter/split
shifts every row after it, so a request can decode with another
request's - or no - sampler and processors. Under concurrent mixed load
this silently disables grammar constraints (json_schema responses come
back as prose) and thinking budgets (unbounded reasoning), with no error
anywhere.

Record at insert time what each uid must run, and realign the positional
lists from that registry at the step chokepoint - the same place as the
jundot#1799 normalisation. A rate-limited warning fires whenever a drift is
actually corrected, so the silent corruption becomes observable. The
registry is a bounded OrderedDict so a missed cleanup can never grow it
unbounded.

Repro (concurrent plain + constrained request, 0.3s apart): 10/10
thinking_budget violations and 5/5 grammar violations on main; 0/15 with
this fix. Solo and sequential behavior unchanged, byte-identical at
temperature 0.
efortin added a commit to efortin/omlx that referenced this pull request Jun 13, 2026
jundot#1799 made the generation step crash-safe by normalising None row slots,
but the positional drift behind it was still there: a stale or offset
slot left in samplers/logits_processors by batch extend/filter/split
shifts every row after it, so a request can decode with another
request's - or no - sampler and processors. Under concurrent mixed load
this silently disables grammar constraints (json_schema responses come
back as prose) and thinking budgets (unbounded reasoning), with no error
anywhere.

Record at insert time what each uid must run, and realign the positional
lists from that registry at the step chokepoint - the same place as the
jundot#1799 normalisation. A rate-limited warning fires whenever a drift is
actually corrected, so the silent corruption becomes observable. The
registry is a bounded OrderedDict so a missed cleanup can never grow it
unbounded.

Repro (concurrent plain + constrained request, 0.3s apart): 10/10
thinking_budget violations and 5/5 grammar violations on main; 0/15 with
this fix. Solo and sequential behavior unchanged, byte-identical at
temperature 0.
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.

Concurrent plain + structured requests crash: GenerationBatch.extend() re-introduces logits_processors None row slots (gap left by #1747)

2 participants