Repository navigation
TASK: merge upstream/main 6bba484f (v0.32.15+3) — parser-deadlock fix, metadata cache, MLX/llama bumps - #208
Conversation
Temporarily carry ml-explore/mlx-c#127
…llama#17883) When a builtin parser rejects model output, the completion callback wrote the error to an unbuffered channel and returned. The callback cannot stop generation -- it has no error return -- so the next chunk re-entered the callback, hit the same parse error and blocked writing to a channel the consumer had already stopped reading after emitting its 500. The completion never returned, the goroutine leaked and the runner request was never released, so retrying the same prompt hung with no log output until the client gave up. Record the parse error, cancel the completion, and report it once the completion has returned. Parse failures landing on the final chunk were already terminal, which is why non-thinking requests and the direct qwen3-coder parser path failed cleanly and only thinking mode wedged. ChatHandler and GenerateHandler share the defect: both run the same parser in the same shape of callback behind a consumer that stops reading at the first error. GenerateHandler had no cancel func at all, so one is added there. Fixes ollama#17825
The default packaging was broken due to mac assumptions leaking into windows
Ten upstream commits since merge-base d67ad83, verified by dry-run merge-tree: eight auto-merge, two files conflict (server/routes.go in GenerateHandler where the marker flow lives; x/mlxrunner/mlx/CMakeLists.txt where both sides fixed the same RPATH bug). Headline item is upstream e0c95a5, the mid-stream parser-error deadlock fix. Task doc carries the full conflict analysis, resolution guidance, and the verification gate order (server+renderer tests, vision goldens, preflight). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewing as consolidator. This is a good handoff — the dry-run-before-the-merge shape is right, and the two conflicts are correctly identified. One thing must be added to the acceptance criteria before anyone acts on it. Add a build gate, and do not let a red check be clicked pastThe last upstream sync, #165, merged with
That is this operation, and the failure class is specific to it: a sync that keeps both copies of a moved symbol reads as a plain addition in review and is invisible to a hunk-level eye. Your criterion 2 scopes tests to Concretely, I would add as criterion 2a:
And the process half, which no criterion can enforce: if the On the conflicts themselvesConflict 1 is the one I can speak to directly: Conflict 2's resolution rule — parser errors set Criterion 4 is the right one to not skipPreflight against a benchmark host before deploy: |
13 upstream commits, 2026-08-18 -> 2026-08-20. Assessment and handoff: docs/maxusai/tasks/upstream-sync-2026-08-21.md. Two conflicts, both predicted by the dry run, resolved as specified there. x/mlxrunner/mlx/CMakeLists.txt -- both sides fixed the same bug (the Mach-O @loader_path RPATH spelling written on ELF). Kept ours: the comment explaining the failure, and the stricter if(APPLE)/elseif(UNIX) guard over upstream's if/else. Upstream's sibling dynamic.c change (Windows SetDllDirectoryA + LoadLibraryExA DLL-search fix) auto-merged and is kept. server/routes.go -- both hunks land in GenerateHandler, on the fork's structured-outputs marker-flow state machine. Kept our pass-one/pass-two loop and threaded upstream's e0c95a5 parser-error fix through it. A builtin parser rejecting output mid-stream used to write the error to ch from inside the completion callback; the callback cannot stop generation, so the next chunk re-entered, hit the same error, and blocked forever on a channel the consumer stopped reading after its 500 -- leaking the goroutine and never releasing the runner. Both builtinParser.Add sites now record parserErr and cancel() instead. Two gates the flat upstream handler does not need: - the post-completion error branch skips reporting when parserErr is set, so a parse-induced cancel is not mistaken for a runner fault and does not reach expireRunnersForRuntimeOOM; - the pass-two restart is held behind parserErr == nil. The marker flow sets state = ReadyToApply *before* parsing the closed thinking, so without this a parse failure there would leave pass two armed and restart generation on an error. Verified: go build ./server/ ./model/... ./llm/ ./api/, and go test ./server/ ./model/renderers/ ./model/parsers/ green -- including upstream's new TestGenerateParseErrorMidStreamDoesNotWedge and TestChatParseErrorMidStreamDoesNotWedge, and the fork's TestSchedAbandonedEvictionDoesNotBlockQueue, TestSchedAbandonedOOMEvictAllDoesNotBlockQueue and TestQwen38ReasoningEffortMapping (unchanged by b8a6272, which only re-renders multi-system / developer-role conversations). PROVENANCE: server_version (SPEC H11) is the comparability boundary for any benchmark cell measured on a build from this merge. a5165c5 adds a model metadata cache keyed by manifest digest, which lowers the per-request latency floor, and b8a6272 changes qwen3.8 system-message rendering. MLX pin adf21dea -> 27fec909 (two bumps) and llama.cpp b10434 -> b10488 also land here, so numeric vision results are not comparable across this commit without a re-baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merge done on this branch; acceptance criteria workedMerge commit 1. Merge — done
2. Tests — green
3. MLX vision goldens — 31b passes, 12b/26b unrunnable here
The MLX bump did not move gemma4 vision numerics. 4. Preflight — FAIL=1, PASS=17, and the one failure is the point
Every behavioural assertion passes on the new payload; the ladders did not move. The sole failure is the identity pin refusing to let ladders measured on b10434 imply a pass on b10488 — exactly what it exists for. So this build is not deploy-validated yet. Per the harness README the fix is a deliberate re-baseline: run Patch 5. Provenance — doneThe merge commit body records Not done
|
…2.15 sync The upstream sync moved llama.cpp b10434 -> b10488, so payload_pin failed on the merged build: expected 7e4c0a968, actual 9d77fa172. That is the check working -- it refuses to let ladders measured on one payload imply a pass on another. Re-measured before changing the pin, not after. measure_ladder.py against 0.32.14-dynres-108-g76918a7 returned byte-identical rows for all three arches: nemotron_h_omni [266, 266, 578, 2306, 3270] budgets 256/3328 stride 32 gemma4 [1102 x 5] budgets 70/1120 stride 48 qwen35 [1034, 1034, 1034, 2306, 4082] budgets 1024/4096 stride 32 Same budgets, same pixel windows, same strides as the b10434 rows. So no expected value is edited here -- only the payload identity, plus the provenance explaining why. The prose in the expect blocks (ADR 0011 rule 4 discussion on qwen35, pinned_not_applicable, the B8 prefix notes) is left intact rather than overwritten with generator output that does not carry it. Not a new profile: the README's "add a new [profiles.<id>]" path is for a changed patchset or a new platform. The patchset (001/002/004/005/903) still describes this build exactly, and resolve_profile keys only on (platform, version) -- two cuda profiles sharing the 0.32.x-dynres pattern would resolve by file order, which is worse than useless. 903 is still required: ggml-org/llama.cpp#27044 re-checked 2026-08-21, still open, so b10488 carries the MMQ ids-path defect exactly as b10434 did. Verified: test_verdicts.py 43/43, and the full preflight (including the pinned-budget probe) on the merged build is now PASS=18 SKIP=2 FAIL=0, against FAIL=1 before this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All five acceptance criteria now greenFollow-up to the previous comment. The two gaps it flagged are closed. The models were there all along — on a different disk
So the store is 3. MLX vision goldens — all three arches PASS
The MLX pin bump moved nothing on any of the three. That was the numeric risk this criterion existed to catch, and it is clear. 4. Preflight — PASS=18, SKIP=2, FAIL=0Re-baselined in
Two judgement calls worth review:
Status
Branch |
Handoff PR — the merge is not done yet. This branch carries the full assessment in
docs/maxusai/tasks/upstream-sync-2026-08-21.md. The agent picking this up should perform the merge on this branch so this PR closes with the work.Scope
Merge
upstream/mainat6bba484f(v0.32.15-3-g6bba484f, fetched 2026-08-21) intomain. Merge-base isd67ad834; exactly 10 upstream commits are in scope (2026-08-18 → 2026-08-20). A dry-rungit merge-tree --write-tree main upstream/mainshows 8 of 10 auto-merge; two files conflict — everything below is from that dry run.Why merge
e0c95a5f— mid-stream parser-error deadlock fix (the reason to do this promptly). A builtin-parser rejection mid-stream leaks the completion goroutine and never releases the runner request; retries hang silently. Only thinking mode wedged. Our fork carries its own parser changes (nemotron3nano, qwen35) and runs hours-long thinking-mode benchmark campaigns — exactly the exposure. Ships withserver/routes_parse_error_test.go.a5165c53— model metadata cache keyed by manifest digest with singleflight; lowers the per-request latency floor under our req/h numbers. Provenance-relevant; covered by SPEC H11 per-cellserver_version.adf21dea…→27fec909…(two bumps + mlx-c 0.32.1 regen compat patch) and llama.cppb10434→b10488— routine, but the numeric risk to the gemma4 MLX vision path gates on the golden tests below.4e134213(mac-assumption fix — we already fixed the RPATH half in-fork; their WindowsLoadLibraryExAhalf is new and auto-merges),b8a62724(qwen3.8 system-message folding — single-system-prompt probes explicitly unaffected),d1bd15cc(Dockerfile),b7871fc0(desktop app only),6bba484f(lint).The two conflicts
x/mlxrunner/mlx/CMakeLists.txt— trivial. Both sides fixed the same Mach-O-RPATH-on-ELF bug; both land on$ORIGIN. Keep ours (elseif(UNIX)+ explanatory comment).server/routes.go— contained but real. Two conflict regions, both inGenerateHandler, exactly where the fork's structured-outputs marker flow lives (pass-1/pass-2, ADR 0010 transition metrics). The ChatHandler half of upstream's fix auto-merges. Resolution: thread upstream'sparserErr+cancel()pattern through our extended callback — parser errors must setparserErrand cancel instead of writing tochfrom inside the callback; report once after the completion returns; keep ourexpireRunnersForRuntimeOOMcall gated onparserErr == nil; a pass-one parse error must not leave the pass-two restart armed. Full hunk-level detail in the task doc.Acceptance criteria (in order)
go test ./server/ ./model/renderers/ ./model/parsers/green — including upstream's newroutes_parse_error_test.goand ourroutes_generate_test.go,sched_headofline_test.go,qwen38_effort_test.go. (Known baseline:go build ./...fails on the app/dist embed — pre-existing; test the listed packages, not./....)x/mlxrunner/vision_golden_test.go, 12b/26b/31b goldens) — single-owner-thread rule applies.docs/maxusai/vision-suite/preflight/) against a benchmark host before any deploy.server_versionis the comparability boundary for benchmark cells measured on the new build.🤖 Generated with Claude Code