Repository navigation
server: chat transition restarts require a deferred pass one - #238
Merged
Merged
Conversation
ChatHandler's two thinking→content transition sites gated only on formatConstrains, while GenerateHandler's twin has always gated on deferring: with think:false on a thinking-capable parser (forceImmediate), pass one already ran WITH the format applied, and a parser that surfaces thinking anyway made the transition cancel it and run a second constrained pass — the request billed twice to apply nothing, with reconstructed metrics (pass1 is nil). Found by the PR #233 max-effort review (verdict PLAUSIBLE, pre-existing). Reachability is exactly the Init contract: in-tree thinking parsers suppress thinking under Init(think:false) — gemma4 discards channel events, pinned by the thinking-leak row in TestChatFormatPassthrough — so the double pass needs a parser that ignores the think value, which nothing forces on a registry-loaded one. TestChatTransitionRequiresDeferring registers such a parser and pins the guard: RED reproduced the double call (got 2), GREEN passes with the format applied on the single pass. Evaluated and rejected: Generate's !deferViaMarker exclusion. Chat marker models keep the transition deliberately — the injected-stop resume is their primary path, but TestChatWithPromptEndingInThinkTag pins the transition as the fallback when the close marker streams through without tripping the stop (a first guard attempt with the exclusion hung that test); both paths consume state None, so whichever fires first owns pass two. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
glennneuber
added a commit
that referenced
this pull request
Sep 4, 2026
Gates 1 and 2 of the v0.33.3 fold: the mechanical merge and the no-GPU test set. 28 conflicts, resolved per docs/maxusai/tasks/upstream-sync-2026-09-04.md and Glenn's decisions D1-A and D2. Payload pins take upstream's values: LLAMA_CPP_VERSION b10760, MLX_VERSION 37c26e5755da..., MLX_C_VERSION c74db5307cc8... (the mlx/compat/ carry patch is deleted with upstream's bindings regeneration). D1-A — the gemma4 MLX package stays the fork's --------------------------------------------- media.go / vision.go / media_test.go / vision_test.go: ours whole-file (add/add). gemma4.go: ours, taking only upstream's package doc line — every other upstream hunk there is vision/audio wiring (parseMultimodalConfig, buildMasks threaded through DecoderLayer/Attention, the media-placeholder embed masking, loadAudioWeights) and is incompatible with the fork's visionChunkMask design. gemma4_test.go / gemma4_moe_test.go: upstream's (pure mlxtest port, no vision content); vision_test.go's nine useMLXTestThread sites ported to mlxtest.Run since upstream deleted that helper with gemma4_moe_test.go. Excluded under D1-A (they reference upstream media.go symbols the fork does not define — multimodalConfig, MultimodalEmbedder, makeClippableLinear, m.MM / m.Vision / m.Audio, visionSoftTokenBudget — and do not compile here): x/models/gemma4/audio.go x/models/gemma4/audio_test.go x/models/gemma4/process_image.go Kept, and dead under D1-A: x/models/gemma4/process_audio.go and process_audio_test.go compile standalone (they depend only on the new x/mlxrunner/model/audio package), so they stay as the audio front-end a D1-B spike would need. x/mlxrunner/model/audio is a clean add and is likewise unused. isGemma4Renderer is restored in server/renderer_resolution.go: upstream deleted it when it started advertising gemma4 audio, and this fork ships gemma4 vision without audio, so server/images.go's suppressAudioCapability keeps its gemma4 branch (the auto-merge had silently dropped it) and server/model_list_cache.go keeps its mirror. Upstream's removal of the gemma4 *vision* suppression is taken in both files; server/images_test.go keeps the fork's "keeps vision, suppresses audio" cases. The per-request budget path is unchanged and asserted at compile time (gemma4/media.go's `_ base.MediaBudgetModel = (*Model)(nil)`): PrepareMediaWithBudget, resolveImageBudget, base.MediaBudgetModel and x/mlxrunner/media.go's bm.PrepareMediaWithBudget dispatch all survive, so image_max_tokens cannot fall through to PrepareMedia for gemma4. D2 — ADR 0010's arithmetic on upstream's plumbing ------------------------------------------------ Upstream's shapes are kept everywhere: the IncludeIntermediateMetrics request field (api/llm/mlxrunner client + pipeline), ChatHandler's `includeIntermediateMetrics := req.Format != nil && currentFormat == nil`, the firstPassMetrics capture with its non-terminal blanking, and the PromptEvalCachedCount pass-through (routes both handlers, llama_server completion + chat, mlxrunner client, pipeline's `len(session.inputs) - len(session.remaining)`). The fold stays the fork's. Upstream's `else if Applying && r.Done` block is dropped because ADR 0004's pass1 summing below it does the same job; running both would count pass one twice. What upstream's plumbing buys is better inputs, not a different fold: at the ChatHandler transition site pass one now has a report of its own (every chunk of a deferring pass carries metrics), so reportedPassMetrics() prefers it over ADR 0010's textual reconstruction — that report is the runner's cache-inclusive prefill, image-embedding tokens included, which is exactly what ADR 0010's subtraction exists to recover. transitionPassMetrics()/transitionPromptDelta remain as the fallback for a runner that reports nothing, and the delta is still computed only from a reconstructed pass (it needs the textual count on both sides). PR #238's `deferring` gate is kept on both ChatHandler transition sites, and llm/llama_server.go keeps result.final, applyCompletionFormat, visionServerArgs, kvCacheFlagValues and ggmlCublasComputeTypeEnv. One assertion in upstream's new restart test is re-expressed for the fork (server/routes_generate_test.go, wantMetrics): upstream reclassifies the continuation's prefill as generation work, so it reports prompt duration as pass one's alone and folds pass two's prefill into eval duration. The fork keeps ADR 0004's summing — each pass's prefill is prefill, each pass's decode is decode. Counts, cached count and eval count now match upstream's expectations exactly; only that duration split differs, and adopting upstream's would move every recorded think+format cell's tok/s, which is a measured surface and a decision of its own. New test TestChatThinkFormatTransitionMetricsReportedPassOne asserts the folded count on a vision-shaped request: it fails if the fold ever falls back to a textual count that cannot see the image surplus. MLX bindings and the test harness --------------------------------- x/mlxrunner/mlx/{mlx,stream}.go: upstream's single-buffer error contract (lastError, mlxError[T], mlxCheck[T]). ClaimOSThread and the `__thread _mlx_thread_owned` flag are deleted, with their callers in x/create/mlxthread.go, x/mlxrunner/server.go and the two vision oracle tests. ADR 0017's guarantee is re-expressed on x/internal/mlxthread, whose Start locks the worker goroutine and deliberately never unlocks; the ADR carries a status amendment saying so, and AGENTS.md / docs/development.md now point at mlxthread and mlxtest.Run. memory.go's MemoryLimit / SetMemoryLimit / SetCacheLimit are ported off mlxCall onto mlxError — mlx_get_memory_limit, mlx_set_memory_limit and mlx_set_cache_limit all survive the MLX-C regeneration with unchanged signatures (x/mlxrunner/mlx/generated.h:5301-5316, include/mlx/c/memory.h:34-38). x/mlxrunner/client.go is mechanical: mlxRunnerEnvDefaults, CacheThrashingCheckEnv, budgetWithOverride and MemoryLimitEnv are untouched. x/mlxrunner/pipeline.go keeps every deferred cleanup on guardClose and keeps the stopper block, with upstream's cachedPromptCount and per-chunk metric enrichment folded in. x/internal/mlxtest: upstream's Run / RunSubtest / SkipIfUnavailable. Of the 18 mlxtest.Setup sites, 17 were in upstream-owned files and upstream ported them itself; the two vision oracles (vision_golden_test.go, vision_e2e_test.go) already drive mlxthread.Start and only lost their ClaimOSThread line — both still compile and skip cleanly without a payload. x/mlxrunner/mlx/memory_test.go's fork-only SetCacheLimit test is ported onto upstream's withMLXThread(t, func(*mlxthreadtest.T)) signature. x/mlxrunner/constrain_bench_test.go and constrain_test.go are DELETED rather than ported: the bench calls mlxtest.Setup(b) and the new API takes only *testing.T, and constrain_test.go's skipIfNoMLX came from an upstream file that no longer defines it. Both drive MLX from a plain test goroutine, which is invalid under the shared-thread model. This brings forward part of ADR 0033's stated follow-up; constrain.go itself and the four non-MLX constrain tests are left for that PR. Build, CI and docs ------------------ cmake/mlx/CMakeLists.txt is the union: the fork's $ORIGIN RPATH block and quadmath in MLX_INCLUDE_REGEXES, upstream's cusolver / cusparse / nv[Jj]it[Ll]ink, OLLAMA_LIB_DIR destinations and license installs. .github/workflows/test.yaml is the union of the fork's path filters (as #232) with upstream's go_mod_changed filter, go_license job and MLX Darwin payload steps. Dockerfile auto-merged correctly — upstream's COPY mlx mlx removal and go-license step, the fork's ccache block. llama/compat: upstream's re-cut 001-llama-cpp-hooks.patch wins verbatim, 002/004/005/801/903 are untouched, and the README merged upstream's b10729 load_data_range paragraph into the fork's band text. docs/api.md takes upstream's prompt_eval_cached_count line and its reworded prompt_eval_duration, and keeps the fork's eval_count note; the fork's image_min_tokens / image_max_tokens and kv-cache-type docs live in docs/maxusai/ and were never in docs/api.md, so nothing was re-added. Gate 2 (golang:1.26.0, -u 1000:1000, caches on the 8 TB array) -------------------------------------------------------------- go build ./... PASS go vet ./... 1 finding, pre-existing go test ./llm/ -run 'TestImageTokensForSize|TestKVCacheType' PASS go test ./server/ ./model/... ./llm/ ./api/ ./convert/ \ ./x/structured/... ./x/mlxrunner/... ./x/internal/... \ ./x/models/... ./x/create/... PASS 4230 pass, 0 fail, 237 skip python3 docs/maxusai/vision-suite/preflight/test_verdicts.py 94 tests OK, 6 skipped (quality arm, pre-existing) The vet finding is tokenizer/bytepairencoding_test.go:542:5 "result of slices.Collect call not used". It is not this merge's: the file is byte-identical to v0.33.3's and unchanged since before the merge base, and a control `go vet ./...` on origin/main in the same image reports the same single finding. No new vet findings. 188 of the 237 skips are "MLX not available: failed to load MLX dynamic library" — the golang image has no native payload — and a skip is not a pass: every MLX kernel path, including TestVisionGoldenParity and TestVisionEndToEnd (which also gate on OLLAMA_VISION_E2E), is unexercised here and is Gate 4's job. gofumpt -l over the 120 Go files this merge touched lists one: server/routes_generate_test.go, whose two hunks are pre-existing fork code in TestChatFormatPassthrough's table (identical hunks on origin/main), outside anything changed here; left alone rather than smuggling unrelated reformatting into a merge commit. SPEC H11 -------- Once the tag is cut, this fold's build identity is 0.33.3-dynres-0-g<this merge's sha>. That is a NEW build identity, not an equivalence to any 0.33.2 stamp: the payload moved (b10630 -> b10760) and the MLX/MLX-C pins moved with it, so no 0.33.2 measurement carries over and nothing recorded here may be folded into a 0.33.2 table (ADR 0032, ADR 0011 rule 5). Not verified here: no native build, no GPU, no preflight. Gates 3-6 stand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #233's max-effort review (the one PLAUSIBLE finding deliberately skipped there, pre-existing).
The bug: ChatHandler's two thinking→content transition sites gated only on
formatConstrains, while GenerateHandler's twin gates ondeferring. With think:false on a thinking-capable parser (forceImmediate), pass one already ran with the format applied — a parser that surfaces thinking anyway made the transition cancel the pass and run a second constrained generation: double token cost to apply nothing, metrics reconstructed (pass1 nil).Reachability, measured: in-tree parsers suppress thinking under
Init(think:false)(gemma4 discards channel events — now pinned by a thinking-leak row inTestChatFormatPassthrough), so the live exposure is a parser that ignores the think value — which nothing forces on a registry-loaded one.TestChatTransitionRequiresDeferringregisters exactly such a parser: RED reproduced the double call (got 2), GREEN passes single-call with the format applied from pass one.Marker exclusion evaluated and rejected: a first guard including Generate's
!deferViaMarkerequivalent hungTestChatWithPromptEndingInThinkTag/structured_outputs_restart_non-stream— chat deliberately keeps the transition as the fallback for marker models whose close marker streams through without tripping the injected stop; both paths consumestructuredOutputsState_None, so whichever fires first owns pass two. Documented at the site.go test ./server/ -count=1green (5.6s, full package).🤖 Generated with Claude Code