diff --git a/CLAUDE.md b/CLAUDE.md index 9b0c2123..fab124e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -675,6 +675,7 @@ Current patches: | `0010-server-cast-vocab-type-for-common-json.patch` | **Upstream regression from the b10585 `common_json` switch (#27511), one line.** `get_res_model_info()` (`tools/server/server-context.cpp`) builds the `GET /models` + `GET /v1/models` payload and emits `{"vocab_type", meta.model_vocab_type}` — an **unscoped enum**. `common_json_value`'s integral constructor template is `std::is_integral`-gated, which *excludes* enums, so the value binds to `common_json_value(bool)` and serialises as `true`/`false` instead of the numeric vocab type. It was correct while the alias was `nlohmann::ordered_json` (nlohmann serialises an enum as an integer), so upstream regressed it silently when they flipped the alias. The project ships this: `server-context.cpp` is compiled into `libjllama` and both routes are served by `NativeServer` — the default fat-jar `Main-Class` — in full **and** attach mode (`patches/0007`'s common route table registers them). The patch casts the value to `int` at the emit site, mirroring what `jllama.cpp` does for its own two `"vocab_type"` sites. Upstream-submittable; **not yet filed upstream**. Applies after `0002`/`0003` (same file) — numbered `0010` because `0009` is burned: it names the subprocess.h patch dropped at the b10280 bump (see the note below this table), and reusing the number would make that note read as if it were about this patch. **On every bump, check whether upstream cast the value themselves; if they did, DROP this patch rather than refreshing it** — the fail-loud applier only detects "does not apply", never "upstream already fixed this", and no test can catch a redundant carry here because `get_res_model_info` is `static` inside `server-context.cpp` and unreachable from `jllama_test`. See the `CommonJsonEnumTrap` tests in `test_json_helpers.cpp` for the mechanism the cast defends against. | | `0011-peg-parser-lenient-invalid-utf8.patch` | **A model that emits one malformed UTF-8 byte turns a finished generation into an HTTP 500.** The server parses *every* completion through `common_chat_parse()`; with no chat parser configured (plain `/completion`) that is the content-only fallback `content(rest()) + end()`, whose scan is `common_peg_until_parser` (`common/peg-parser.cpp`). `common_chat_peg_parse()` always parses in **lenient** mode, and that scan tolerates an `INCOMPLETE` trailing UTF-8 sequence by keeping the text before it — but the `INVALID` branch right below it returns `FAIL` unconditionally, ignoring leniency. One stray byte anywhere in the generated text therefore throws `"The model produced output that does not match the expected Content-only format"` and the request 500s even though generation completed normally (`stop processing: n_tokens = 4, truncated = 0`). The patch makes the `INVALID` branch respect `ctx.is_lenient()` exactly like the `INCOMPLETE` branch — keep the text up to the malformed byte — and adds an upstream `tests/peg-parser/test-unicode.cpp` case pinning both the lenient and the still-failing strict behavior. **Strict mode is unchanged**, which is what keeps upstream's own tests green: `tests/peg-parser/test-unicode.cpp` *does* assert `FAIL` on invalid UTF-8 through the *until* parser (a `malformed UTF-8` block with three `p.until("")` cases), but each builds a bare `common_peg_parse_context` with no `COMMON_PEG_PARSE_FLAG_LENIENT`, so the lenient-only change cannot reach them. This patch adds its case inside that same block. Found by `NativeServerAttachIntegrationTest.completion_overHttp_served`, which 500s on all six Java CI platforms. Upstream-submittable; **not yet filed upstream**. Touches only `common/peg-parser.cpp` + that test, which no other patch touches, so it is independent of `0001`/`0006`/`0007`. Runnable guard: the `ContentOnlyParseUtf8` tests in `src/test/cpp/test_utils.cpp` — unlike the upstream test they are compiled and run in CI on every platform, so a bump that drops this patch reds `C++ Tests` instead of one Java job. | | `0006-server-embed-native-server-jni.patch` | **Makes `server.cpp`'s `llama_server` embeddable in the JVM** so the `NativeServer` JNI bridge can run the full upstream HTTP server (WebUI included) inside `libjllama` — see "Two server modes" below. b9870 already exposes `int llama_server(int, char**)` (non-static; no `main` in the file), so the patch only adds embedded-mode support: (1) a `g_llama_server_embedded` flag + `llama_server_set_embedded()` / `llama_server_request_shutdown()` (declared in the committed `src/main/cpp/native_server_bridge.h`); (2) skips installing the process-wide SIGINT/SIGTERM handlers when embedded (they would hijack the JVM's); (3) in embedded mode parses the **forwarded** argv via `common_params_parse` instead of `common_params_parse_main` (whose `GetCommandLineW` recovery would pick up `java.exe`'s command line — the same Windows class of bug `0001` fixes). `llama_server_request_shutdown()` mirrors the SIGTERM path (invokes the installed `shutdown_handler` → `ctx_server.terminate()` unblocks `start_loop()`), giving JNI an out-of-band stop since `ctx_server` is loop-local. Applies **after `0001`** (which flips this call site to `common_params_parse_main`), so its context is the post-`0001` tree; regenerate against `0001`+source on a bump. Only touches `tools/server/server.cpp`. | +| `0012-model-guard-zero-split-sum-and-name-the-device-index.patch` | **A GPU that reports zero free memory makes every model load fail with the unactionable `error loading model: vector`.** `llama_model_base::load_tensors` (`src/llama-model.cpp`) weights the per-device layer split by `ggml_backend_dev_memory()`'s `free`, then normalises: `splits[i] /= split_sum`. With a single device reporting `free == 0` that is `0/0` → **NaN** in every split point; NaN compares false against everything, so the `std::upper_bound` below returns the end iterator, `layer_gpu == n_devices()`, and `devices.at(layer_gpu)` throws `std::out_of_range` — whose libc++ `what()` is the bare string `"vector"`, which `llama.cpp`'s `catch (const std::exception &)` prints verbatim. Upstream's `free == 0 && total == 0` host-memory fallback does **not** fire, because `total` is `recommendedMaxWorkingSetSize` and is non-zero. **Reachable since b10618..b10797**: upstream `8c0b9cd04` ("metal : fix memory query under low-memory conditions", [#27701](https://github.com/ggml-org/llama.cpp/pull/27701)) changed `ggml-metal-device.m` to `*free = *total > cur ? *total - cur : 0`; before that clamp an over-committed device (`currentAllocatedSize > recommendedMaxWorkingSetSize`) *underflowed* to a huge `size_t`, which normalised fine, so the same precondition was harmless. That is why the `Java Tests macOS …` jobs went red at the b10792→b10797 step while every Linux/Windows job stayed green — **and why only a GPU build can fail this way at all**: `act_gpu_layers` is `devices.empty() ? 0 : …`, so with no GPU backend `devices` is empty, every layer returns early on `cpu_dev`, and the `.at()` line is unreachable. **Shape:** the two blocks are lifted out of `load_tensors` into free functions declared in `src/llama-model.h`, purely so they can be driven by a test — the failing state needs a real over-committed GPU and cannot be arranged through any public API. `llama_model_splits_normalize()` carries **the fix**: on `split_sum == 0` it `LLAMA_LOG_WARN`s and falls back to an even split (`splits[i] = float(i+1)/splits.size()`), the only neutral choice when no device can be preferred and exactly right for a single device. `llama_model_splits_select_device()` carries **the diagnostic**: it bounds-checks the index and throws a `std::runtime_error` naming the function, the offloaded layer, the device index, the split-point count **and the split points themselves** — with NaN splits that message prints `nan` and names the cause outright, which is precisely what was missing when this had to be diagnosed by reading source. **A second, backend-independent trigger reaches the same line**, found while writing this up and verified against the unfixed library: `--tensor-split` values are parsed with `std::stof` and never range-checked (`common/arg.cpp`), so `-ts 1,-1` cancels out, `split_sum` is 0 again, the split points become `[inf, -nan]`, and every layer maps one past the last device — on CUDA, Vulkan or ROCm just as much as on Metal, with no memory pressure involved. That is what makes this an ordinary upstream defect rather than a Metal edge case, and the warning names both causes rather than only the memory one. Also adds upstream `tests/test-model-split.cpp` (5 cases in upstream's `testing.h` style) + its `llama_build_and_test` registration. Touches `src/llama-model.{cpp,h}`, `tests/test-model-split.cpp` and `tests/CMakeLists.txt` — **none** of which any other patch touches, so it is independent of all of them. Upstream-submittable ("model: fall back to an even split when no device reports free memory"); **not yet filed upstream**. **Runnable guard: `src/test/cpp/test_model_split.cpp`** — a FetchContent subproject builds with `LLAMA_BUILD_TESTS=OFF`, so the upstream test above is applied-but-never-compiled here (same as `0001`'s test). That file drives the same two functions from `jllama_test`, which runs on **every** platform in `C++ Tests`, so a bump that drops this patch fails the build at link time everywhere instead of surfacing as one red macOS Java job. **Verification limit — read before assuming this can be dropped:** the *failing path* still cannot be reached without a GPU backend, so the guard pins the arithmetic (what actually broke), not the end-to-end load; the end-to-end proof is the macOS CI job. On a bump, re-check whether upstream added its own `split_sum == 0` guard (grep `split_sum` in `src/llama-model.cpp`) and **drop this patch rather than refreshing it** if they did — the fail-loud applier detects "does not apply", never "upstream already fixed this". | **`0009` was dropped at the b10280 bump.** Upstream merged [sheredom/subprocess.h#104](https://github.com/sheredom/subprocess.h/pull/104) — the exact fix this @@ -1446,8 +1447,9 @@ ctest --test-dir build --output-on-failure -R "ResultsToJson" | `src/test/cpp/test_jni_helpers.cpp` | 56 | All functions in `jni_helpers.hpp` using a zero-filled `JNINativeInterface_` mock (incl. the `utf8_to_jstring_impl` byte-array string path: emoji byte-preservation, truncated-UTF-8 replace-not-throw) | | `src/test/cpp/test_tts_wav.cpp` | 2 | The in-memory WAV writer `pcm_to_wav16_bytes` in `tts_wav.hpp` (WAV header/payload + little-endian clamping) — our own code, not upstream. The Qwen3-TTS pipeline it pairs with (`mtmd_helper::gen_audio`) is entirely upstream-owned (no project-side DSP to unit-test here). The load path is additionally covered by `test_tts_params.cpp` (3 tests over `tts_params.hpp`'s `build_tts_params`, plus 2 pinning the upstream `-1` default it depends on), which pins the CPU-thread resolution whose absence used to crash the JVM on every platform — see the `TODO.md` entry for the mechanism. End-to-end coverage is `TtsIntegrationTest`, which is model-gated. | | `src/test/cpp/test_tts_params.cpp` | 13 | The **three** builders every hand-assembled `common_params` goes through: `build_tts_params` (`tts_params.hpp`), `build_train_params` (`train_params.hpp`) and the shared `jllama::resolve_cpu_params` (`cpu_params.hpp`). Each builder is guarded separately on purpose — testing the resolver alone does **not** cover its call sites, because `train_engine.cpp` is compiled into `jllama` only, never into `jllama_test`, and `LlamaTrainerIntegrationTest` is gated on `net.ladenthin.llama.train.model`, which no CI job sets. Without these the JVM-abort bug could regress in the trainer on every platform, unseen. | +| `src/test/cpp/test_model_split.cpp` | 7 | The two `load_tensors()` split helpers that `patches/0012` extracts out of llama.cpp's `src/llama-model.cpp` — `llama_model_splits_normalize` (proportional split, single device, and the zero-sum case that used to produce NaN, **and the cancelling `--tensor-split` case** — `-ts 1,-1` reaches the identical line on any backend with no GPU memory pressure at all) and `llama_model_splits_select_device` (every layer maps to a real device index; malformed split points throw a message that names the function, the layer, the index and the split values instead of libc++'s bare `"vector"`). **This is the runnable guard for `0012`**: the patch also ships an upstream `tests/test-model-split.cpp`, but a FetchContent subproject builds with `LLAMA_BUILD_TESTS=OFF`, so that one is applied-but-never-compiled here. This file is the only place the two functions are linked in CI, on every platform — so a bump that drops the patch fails the `C++ Tests` build outright rather than resurfacing as one red macOS Java job. It is the one test file that includes an **internal** upstream header (`llama-model.h`, via the `${llama.cpp_SOURCE_DIR}/src` include dir added for it), which is deliberate: a signature drift should fail loudly at compile time. | -**Current total: 520 tests (all passing).** +**Current total: 527 tests (all passing).** #### Upstream source location (in CMake build tree) @@ -1492,6 +1494,11 @@ so missing stubs are caught immediately rather than silently. - JNI helper → `test_jni_helpers.cpp` - Upstream result type `to_json()` → `test_server.cpp` - `utils.hpp` function or upstream utility → `test_utils.cpp` + - A function one of the local `patches/` adds to llama.cpp → its own file, e.g. + `test_model_split.cpp` for `0012`. Give every patch that introduces a callable a + guard here: upstream tests carried by a patch are **not** compiled (a FetchContent + subproject sets `LLAMA_BUILD_TESTS=OFF`), so this is the only place a dropped patch + reds CI on every platform instead of on whichever job happens to hit it. 2. Add a `TEST(SuiteName, TestName) { ... }` block using GoogleTest macros. 3. Rebuild: `cmake --build build --config Release -j$(nproc)` 4. Run: `ctest --test-dir build --output-on-failure` diff --git a/docs/history/llama-cpp-breaking-changes.md b/docs/history/llama-cpp-breaking-changes.md index 6eefa0ff..a76f656d 100644 --- a/docs/history/llama-cpp-breaking-changes.md +++ b/docs/history/llama-cpp-breaking-changes.md @@ -695,5 +695,6 @@ Used during `llama.cpp` version bumps: when upgrading, scan this file from the r | b10817–b10819 | `ggml/src/ggml-metal/ggml-metal-context.m` (**#28399: one-line memory-leak fix on an early-return path**), `ggml/src/ggml-sycl/fwht.cpp` (#28254: restore Kronecker-product FWHT support and unbreak `test-backend-ops` on SYCL), `tests/test-backend-ops.cpp`. | **No project-source change, and the review surface is empty**: all three changed files are ggml backends or upstream tests, none of which this project links against or compiles into `jllama`. The server contract was re-checked and is byte-identical (request-field set, response keys in both emit forms); `tools/server/`, `common/`, `include/`, `tools/mtmd/` and `ggml/include/` have no change at all in the range. The Metal leak fix sits next to the **macOS-15 failure** and is not a fix for it; the investigation row below supersedes the reasoning recorded here, including the claim that "the no-Metal macOS job fails identically" (there was no no-Metal job). | | b10817–b10819 | patches + upstream verification | **Zero intersection with the patch set**: the 42 files the eight patches touch versus the range's 3 changed files — empty, so no patch context can have moved. The applier was run for real regardless: fresh `rm -rf build && cmake -B build -DBUILD_TESTING=ON`, configure clean, stamp at `head 6a1a922d269908a29cbd4b49c27e6a8e7fd10fae`, all eight hashes recorded. **No chunking question arises**: the full diff is 17 KB across 2 commits, far inside the runbook's 100 KiB threshold, so this is a straight bump *by* the rule rather than an exception to it. | | **macOS-15 failure — investigation, 2026-09-05** | `Java Tests macOS 15 arm64 (Metal)` + `(no Metal)`, run **33861339600** (#897, dispatch, pin b10797) | **The #28323 lead is refuted, and the b10792–b10797 bisect window it rested on was never established.** Read this row before spending time on the earlier suspicion recorded two rows up. **What actually fails.** Two tests, `MemoryManagementTest#testPromptCacheCompleteMissAfterWarmup` and `LlamaModelTest#testSpeculativeDecoding`, both with `LlamaException: could not load model from given file path` over a native `llama_model_load: error loading model: vector`. The recorded symptom ("the draft model") is **half wrong**: the first failure is **codellama-7b.Q2_K.gguf**, the second the AMD-Llama-135m draft — so it is not size-specific. Both are *repeat* loads: the same 7B model had already loaded and generated successfully dozens of times in the same JVM over the preceding ten minutes. `what() == "vector"` is libc++'s message for `std::out_of_range` from `vector::at()` and for `vector`'s `length_error`; the 33 ms elapsed (10.06.078 → 10.06.111) and the position right after the vocab warnings put it in `load_hparams`/`load_tensors`, not in the big allocation phase. **Why #28323 cannot be it — three independent reasons.** `n_expert_used_arr` is a `std::array` read through `operator[]` behind an `il < n_layer_all` guard, so it cannot raise `out_of_range("vector")` (and its failure mode would be `GGML_ABORT`, which aborts rather than throws). Both failing models are dense (`n_expert == 0`), so the two changed call sites in `weight_buft_supported` (`GGML_OP_MUL_MAT_ID`, `GGML_OP_ADD_ID`) are unreachable and the `load_tensors` guard short-circuits on `n_expert > 0` before ever calling it. And the change is a **widening** — `max(il)` ≥ `[0]` — so it cannot narrow a value into an out-of-range one. **The "no Metal" job has never been a no-Metal job**, which invalidates the "both macOS jobs fail identically, so Metal is not involved" reasoning. It passed `-DLLAMA_METAL=OFF`, and upstream's `llama_option_depr(WARNING LLAMA_METAL GGML_METAL)` forwards only `if (${OLD})` — i.e. it acts on `ON` and **silently ignores `OFF`**, leaving `GGML_METAL` at its `APPLE` default of `ON`. CMake emits no "unused variable" warning either, because the variable *is* read; it just has no effect. Both macOS-15 jobs were therefore the same Metal build, which is why their logs are identical (44 `MTL0`/`ggml_metal` lines in the supposedly Metal-free one) and why "identical failure" carried no information. Fixed in this change set by passing `-DGGML_METAL=OFF`; the two `-DLLAMA_METAL_EMBED_LIBRARY=ON` sites are unaffected, since the shim does forward a truthy value. **The bisect window is wrong, and much wider than 5 commits.** The last CI observation of the macOS *Java test* jobs passing is run **33275073456** (#875, the v5.1.0 release dispatch, 2026-08-29) at pin **b10618**. b10731 was never observed: run #887 (dispatch, b10731) failed at `Code style (spotless) + package graph`, which skipped everything downstream — 30 jobs, no `Java Tests macOS` among them — and every other run between #875 and #897 was cancelled. So the regression window is **b10618 → b10797**, ~180 upstream builds, not b10792 → b10797. **Memory pressure is not the discriminator**, checked rather than assumed: the *green* run #875 logged **8** `ggml_metal_log_allocated_size: current allocated size is greater than the recommended max working set size` warnings and more free pages than the red run, which logged **6**. The 7 GB runner is tight in both. **Resolved to Metal by run 33994480652** — see the row below, which carries the experiment this one could only propose. | -| **macOS-15 failure — cause isolated, 2026-09-06** | `Java Tests macOS 15 arm64 (Metal)` vs `(no Metal)`, run **33994480652** (#910, dispatch, pin b10819) | **It is a Metal bug.** The first dispatch after the `-DGGML_METAL=OFF` fix turns the two macOS-15 jobs into a clean single-variable comparison, and they disagree: same runner, same OS, same `-DGGML_NATIVE=OFF`, identical **1742 tests / 11 skipped** on both sides, differing only in `GGML_METAL`. With Metal: **2 errors** — `MemoryManagementTest` 1 of 10 and `LlamaModelTest` 1 of 60, both `llama_model_load: error loading model: vector`, unchanged from #897. Without Metal: **0 errors**, those same classes 10/10 and 60/60, and **zero** `MTL0`/`ggml_metal` lines in the log where the old build had 44 — so the flag fix demonstrably took effect. The standing conclusion "the no-Metal job fails identically, so this is not a Metal problem" was an artifact of the no-op flag and is now retired. **What this does NOT establish:** `Java Tests macOS 14 arm64 (Metal)` is green, but that job differs from the red one in **two** variables at once — OS version *and* `GGML_NATIVE` (macos-14 builds host-native, macos-15 passes `-DGGML_NATIVE=OFF`) — so it cannot narrow "Metal" to "Metal on macOS 15" or to "Metal plus a portable build". Only the macos-15 pair isolates anything. **What it buys.** The bisect window stays b10618 → b10797 in tags, but the *candidate set* collapses from every upstream change to the Metal backend alone (`ggml/src/ggml-metal/**`) — precisely the directory every bump review in this table waved through as "GPU backend, safe to skip". That rule is sound for compile/link breaks and blind to runtime ones; this is the first entry where it cost something. **Still open:** which Metal commit, and whether upstream has since fixed it. The rest of run #910 was green — 51 success, 1 failure, 12 skipped (all downstream of the red job), including `Run PIT mutation tests: success`, which is the first CI confirmation of the PIT gate since the SLF4J classpath fix. | +| **macOS-15 failure — cause isolated, 2026-09-06** | `Java Tests macOS 15 arm64 (Metal)` vs `(no Metal)`, run **33994480652** (#910, dispatch, pin b10819) | **It is a Metal bug.** The first dispatch after the `-DGGML_METAL=OFF` fix turns the two macOS-15 jobs into a clean single-variable comparison, and they disagree: same runner, same OS, same `-DGGML_NATIVE=OFF`, identical **1742 tests / 11 skipped** on both sides, differing only in `GGML_METAL`. With Metal: **2 errors** — `MemoryManagementTest` 1 of 10 and `LlamaModelTest` 1 of 60, both `llama_model_load: error loading model: vector`, unchanged from #897. Without Metal: **0 errors**, those same classes 10/10 and 60/60, and **zero** `MTL0`/`ggml_metal` lines in the log where the old build had 44 — so the flag fix demonstrably took effect. The standing conclusion "the no-Metal job fails identically, so this is not a Metal problem" was an artifact of the no-op flag and is now retired. **What this does NOT establish:** `Java Tests macOS 14 arm64 (Metal)` is green, but that job differs from the red one in **two** variables at once — OS version *and* `GGML_NATIVE` (macos-14 builds host-native, macos-15 passes `-DGGML_NATIVE=OFF`) — so it cannot narrow "Metal" to "Metal on macOS 15" or to "Metal plus a portable build". Only the macos-15 pair isolates anything. **What it buys.** The bisect window stays b10618 → b10797 in tags, but the *candidate set* collapses from every upstream change to the Metal backend alone (`ggml/src/ggml-metal/**`) — precisely the directory every bump review in this table waved through as "GPU backend, safe to skip". That rule is sound for compile/link breaks and blind to runtime ones; this is the first entry where it cost something. **Still open:** which Metal commit, and whether upstream has since fixed it. The rest of run #910 was green — 51 success, 1 failure, 12 skipped (all downstream of the red job), including `Run PIT mutation tests: success`, which is the first CI confirmation of the PIT gate since the SLF4J classpath fix. **Both open questions are answered by the root-cause row at the end of this table**: the commit is `8c0b9cd04` (#27701), and no — upstream has not fixed it (the bare `splits[i] /= split_sum` is still there at b10850), so the fix is carried locally as `patches/0012`. | | b10819–b10850 | `common/arg.cpp` (**additive**: `--log-jsonl` / `--no-log-jsonl`), `common/log.{cpp,h}` (**additive**: `common_log_set_jsonl`, plus a `level_str` helper and a `json.h` include), `common/jinja/{caps,runtime}.cpp`, `tools/server/server-models.{cpp,h}` (**159 lines rewritten**; `request_stop(name)` added to the header), `tools/server/CMakeLists.txt` + `README.md` + `tests/unit/test_router.py`. **`include/`, `ggml/include/` and `tools/mtmd/` are untouched in the range.** | **No project-source change.** The header moves are additive: `common_log_set_jsonl` is new API nothing here calls, and the two new `common_arg` entries are CLI-only. The 159 changed lines in `server-models.cpp` are the router's instance lifecycle, not its argv rendering. **The patch intersection was NOT empty this time** — `common/arg.cpp` (patch `0001`) and `tools/server/server-models.cpp` (patch `0008`) are both in the range, so the applier had to prove the contexts survived rather than a disjoint file list implying it: a fresh `rm -rf build && cmake -B build -DBUILD_TESTING=ON` applied all eight clean and stamped them at head `f114f91f9ed6792cf402437e3874adad98902744`. Server contract re-checked mechanically and byte-identical (request-field set, `set_hard_limits` bounds, response keys in both emit forms). **Chunking, with the figures recorded rather than a verdict asserted:** the full diff is **466 KB over 31 commits**; excluding the paths this table's earlier rows excluded (`ggml/src`, `tools/ui`, `docs`, `.github`, `conversion`) it is still **171 KB**, over the runbook's 100 KiB threshold — but that figure is misleading, because the remainder is dominated by `scripts/ui-assets.cmake`, `tests/test-backend-ops.cpp`, a new model architecture (`src/models/spark2-5.cpp` + its Jinja template) and `gguf-py/`, none of which the project links against and none of which the old exclusion list names. The **review surface proper** — `common/`, `include/`, `tools/server/`, `tools/mtmd/`, `ggml/include/`, top-level `CMakeLists.txt` — is **27 KB / 10 files / +181 / −98**, comfortably inside the threshold. Recorded both ways so the call is auditable. **Metal moved in this range too** (`ggml-metal-device.m` +7/−2 and a new 147-line `ggml-metal-tuning.cpp`), which matters only because of the macOS-15 finding in the row below: this bump neither targets nor is known to fix it. | +| **macOS-15 failure — root cause + fix, 2026-09-08** | `src/llama-model.cpp` (upstream) ← `ggml/src/ggml-metal/ggml-metal-device.m` `8c0b9cd04` ([#27701](https://github.com/ggml-org/llama.cpp/pull/27701)); carried as `llama/patches/0012` | **Found by reading, not by bisecting — the bisect was prepared and then not needed.** The chain, each link checked against the source rather than inferred: **(1)** `8c0b9cd04` "metal : fix memory query under low-memory conditions" lies inside the b10618→b10797 window and rewrote `ggml_metal_device_get_memory` to `*free = *total > cur ? *total - cur : 0`. **(2)** Both the green and the red run log `current allocated size is greater than the recommended max working set size`, i.e. `cur > total` — so that condition is the **precondition, not the discriminator**; it held on both sides of the regression. **(3)** Before the clamp, `*total - cur` **underflowed** to a huge `size_t`, which normalised harmlessly; after it, the device reports exactly `free == 0`. **(4)** `load_tensors`' `if (free == 0 && total == 0)` host-memory fallback does not fire, because `total` is `recommendedMaxWorkingSetSize` and is non-zero. **(5)** `splits[0] = 0` → `split_sum = 0` → `splits[i] /= split_sum` = **NaN**. **(6)** `std::upper_bound(…, NaN)` — every comparison false — returns the end iterator, so `layer_gpu == 1`. **(7)** `devices.at(1)` on a one-element vector throws `std::out_of_range`, whose libc++ `what()` is the bare string `"vector"`; `llama.cpp`'s `catch (const std::exception & err)` prints it verbatim. **(8)** The **discriminator** is `const int act_gpu_layers = devices.empty() ? 0 : …`: without a GPU backend `devices` is empty, every layer returns early on `cpu_dev`, and the `.at()` line is unreachable — which is exactly why only the Metal job failed. This accounts for every observation the two investigation rows above collected: Metal-only; only on *repeat* loads in a long-lived JVM (that is merely how `currentAllocatedSize` grew past the recommended set size, not a precondition of its own); `what() == "vector"`; ~30 ms in, right after the vocab warnings; and the absence of a `hyperparameters:`/`vocabulary:` prefix, since upstream's own rethrows would have added one. **The fix** is `patches/0012`: `llama_model_splits_normalize()` falls back to an even split when the weights sum to zero, and `llama_model_splits_select_device()` bounds-checks the lookup and throws a message naming the function, the layer, the device index and the split points. Both are lifted out of `load_tensors` into free functions **for testability** — the failing state needs a real over-committed GPU and cannot be arranged through any API — with an upstream `tests/test-model-split.cpp` and, because a FetchContent subproject sets `LLAMA_BUILD_TESTS=OFF`, a project-side runnable guard `src/test/cpp/test_model_split.cpp` that links the same two functions into `jllama_test` on every platform. **A second trigger, found while writing the fix up and verified against the unfixed library:** `--tensor-split` is parsed with `std::stof` and never range-checked, so `-ts 1,-1` makes the weights cancel, `split_sum` is 0 again, the split points become `[inf, -nan]`, and every layer maps one past the last device — on CUDA, Vulkan or ROCm as much as on Metal, in a fresh process with no memory pressure. The macOS failure is therefore one *instance* of a general defect, not a Metal edge case, which is what settles the question of upstream-submittability. **Two lessons worth carrying.** The `ggml/src/**` "safe to skip" rule in the review list is sound for compile/link breaks and blind to runtime ones; this is the first entry where it cost something, and the cost was ~180 builds of bisect window. And a `catch (…) { log(err.what()); }` over a library that throws `std::out_of_range` is a **diagnostic dead end** on libc++, which reports it as `"vector"` and nothing else — the second half of `0012` exists for that reason alone, and is why the message a future occurrence produces will name its own cause. | diff --git a/llama/CMakeLists.txt b/llama/CMakeLists.txt index 22029106..0b107226 100644 --- a/llama/CMakeLists.txt +++ b/llama/CMakeLists.txt @@ -568,6 +568,7 @@ if(BUILD_TESTING) src/test/cpp/test_log_helpers.cpp src/test/cpp/test_tts_wav.cpp src/test/cpp/test_tts_params.cpp + src/test/cpp/test_model_split.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-common.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-chat.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-context.cpp @@ -585,6 +586,11 @@ if(BUILD_TESTING) # jni.h / jni_md.h needed by jni_helpers.hpp (mock JNI tests, no JVM required) ${JNI_INCLUDE_DIRS} ${llama.cpp_SOURCE_DIR}/tools/server + # llama.cpp's internal src/ -- test_model_split.cpp includes llama-model.h to drive the + # two load_tensors() split helpers that patches/0012 extracts. Upstream's own tests do the + # same (tests/test-batch-alloc.cpp, tests/test-quantize-stats.cpp include "../src/..."). + # No header name in that directory collides with tools/server, tools/mtmd or src/main/cpp. + ${llama.cpp_SOURCE_DIR}/src ) target_link_libraries(jllama_test PRIVATE llama-common mtmd llama nlohmann_json GTest::gtest_main) target_compile_features(jllama_test PRIVATE cxx_std_17) diff --git a/llama/patches/0012-model-guard-zero-split-sum-and-name-the-device-index.patch b/llama/patches/0012-model-guard-zero-split-sum-and-name-the-device-index.patch new file mode 100644 index 00000000..43c193ff --- /dev/null +++ b/llama/patches/0012-model-guard-zero-split-sum-and-name-the-device-index.patch @@ -0,0 +1,271 @@ +diff --git a/src/llama-model.cpp b/src/llama-model.cpp +index ffedf89e6..97ef25746 100644 +--- a/src/llama-model.cpp ++++ b/src/llama-model.cpp +@@ -1399,6 +1399,75 @@ void llama_model_base::load_vocab(llama_model_loader & ml) { + vocab.load(ml, kv); + } + ++// Turn the per-device weights collected in `splits` (each device's free memory, or the values ++// the user passed to --tensor-split) into cumulative, normalized split points in (0, 1], so that ++// splits[i] is the fraction of the offloaded layers that belongs to devices [0, i]. ++// ++// Split out of load_tensors() so it can be unit-tested: the degenerate all-zero case below is ++// only reachable with a real over-committed GPU, which a test cannot arrange through the API. ++void llama_model_splits_normalize(std::vector & splits) { ++ // sum and normalize the splits to get the split points ++ float split_sum = 0.0f; ++ for (size_t i = 0; i < splits.size(); ++i) { ++ split_sum += splits[i]; ++ splits[i] = split_sum; ++ } ++ ++ if (split_sum == 0.0f) { ++ // The weights sum to zero, so there is nothing to weight the layers by. Dividing here would ++ // put a NaN (or an inf) in the split points, and NaN compares false against everything, so ++ // the std::upper_bound in llama_model_splits_select_device() below would return the end ++ // iterator and the lookup would run off the end of the device list. ++ // ++ // Two different inputs reach this, and both are reachable in practice: ++ // ++ // * a device that reports no free memory -- e.g. a Metal device whose currentAllocatedSize ++ // has grown past its recommendedMaxWorkingSetSize reports free == 0 with a non-zero ++ // total, so the `free == 0 && total == 0` host-memory fallback in the caller does not ++ // fire and every weight is 0; ++ // * an explicit --tensor-split whose values cancel out, e.g. `-ts 1,-1`. The values are ++ // parsed with std::stof and never range-checked, so this reaches us on any backend. ++ // ++ // Split evenly instead -- the only neutral choice when no device can be preferred over ++ // another, and exactly right for a single device. ++ LLAMA_LOG_WARN("%s: layer split weights sum to zero " ++ "(no device reported free memory, or --tensor-split cancels out), " ++ "splitting layers evenly\n", __func__); ++ for (size_t i = 0; i < splits.size(); ++i) { ++ splits[i] = float(i + 1)/splits.size(); ++ } ++ return; ++ } ++ ++ for (size_t i = 0; i < splits.size(); ++i) { ++ splits[i] /= split_sum; ++ } ++} ++ ++// Map the i_layer-th offloaded layer (of n_layers) to a device index, using the split points ++// produced by llama_model_splits_normalize(). ++int llama_model_splits_select_device(const std::vector & splits, int i_layer, int n_layers) { ++ const int idx = std::upper_bound(splits.begin(), splits.end(), float(i_layer)/n_layers) - splits.begin(); ++ ++ if (idx < 0 || (size_t) idx >= splits.size()) { ++ // Unreachable for well-formed split points, but indexing the device list with an ++ // out-of-range value gives std::out_of_range, whose libc++ what() is the bare string ++ // "vector" -- a message that names neither the container, nor the index, nor this ++ // function, and that the model-load error path prints verbatim. Say what went wrong, ++ // and print the split points, since a malformed one (NaN, unsorted, > 1) is the cause. ++ std::string s; ++ for (size_t i = 0; i < splits.size(); ++i) { ++ s += (i == 0 ? "" : ", ") + std::to_string(splits[i]); ++ } ++ throw std::runtime_error(format( ++ "%s: offloaded layer %d of %d mapped to device index %d, but there are only %zu " ++ "split point(s) [%s] -- the split points are malformed", ++ __func__, i_layer, n_layers, idx, splits.size(), s.c_str())); ++ } ++ ++ return idx; ++} ++ + bool llama_model_base::load_tensors(llama_model_loader & ml) { + const auto & split_mode = params.split_mode; + const bool use_mlock = params.load_mode == LLAMA_LOAD_MODE_MLOCK || params.load_mode == LLAMA_LOAD_MODE_MMAP_MLOCK; +@@ -1466,15 +1535,7 @@ bool llama_model_base::load_tensors(llama_model_loader & ml) { + std::copy(tensor_split, tensor_split + n_devices(), splits.begin()); + } + +- // sum and normalize the splits to get the split points +- float split_sum = 0.0f; +- for (size_t i = 0; i < n_devices(); ++i) { +- split_sum += splits[i]; +- splits[i] = split_sum; +- } +- for (size_t i = 0; i < n_devices(); ++i) { +- splits[i] /= split_sum; +- } ++ llama_model_splits_normalize(splits); + + const int i_gpu_start = std::max(n_layer_all + 1 - n_gpu_layers, 0); + const int act_gpu_layers = devices.empty() ? 0 : std::min(n_gpu_layers, n_layer_all + 1); +@@ -1484,7 +1545,7 @@ bool llama_model_base::load_tensors(llama_model_loader & ml) { + LLAMA_LOG_DEBUG("load_tensors: layer %3d assigned to device %s, is_swa = %d\n", il, ggml_backend_dev_name(cpu_dev), is_swa); + return {cpu_dev, &pimpl->cpu_buft_list}; + } +- const int layer_gpu = std::upper_bound(splits.begin(), splits.begin() + n_devices(), float(il - i_gpu_start)/act_gpu_layers) - splits.begin(); ++ const int layer_gpu = llama_model_splits_select_device(splits, il - i_gpu_start, act_gpu_layers); + auto * dev = devices.at(layer_gpu).dev; + LLAMA_LOG_DEBUG("load_tensors: layer %3d assigned to device %s, is_swa = %d\n", il, ggml_backend_dev_name(dev), is_swa); + return {dev, &pimpl->gpu_buft_list.at(dev)}; +diff --git a/src/llama-model.h b/src/llama-model.h +index 4c4a30e01..dc5560254 100644 +--- a/src/llama-model.h ++++ b/src/llama-model.h +@@ -821,6 +821,11 @@ struct llama_model_base : public llama_model { + + const char * llm_type_name(llm_type type); + ++// load_tensors() helpers, declared here so tests/test-model-split.cpp can drive them directly. ++// See their definitions in llama-model.cpp for what each one guarantees. ++void llama_model_splits_normalize(std::vector & splits); ++int llama_model_splits_select_device(const std::vector & splits, int i_layer, int n_layers); ++ + // convenience macro for loading local variables for load_tensors() in llama_model_base + // note: cast to int64_t since we will use these for the tensor dimensions + #define LLAMA_LOAD_LOCALS \ +diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt +index c46377c76..229cb768d 100644 +--- a/tests/CMakeLists.txt ++++ b/tests/CMakeLists.txt +@@ -160,6 +160,7 @@ if (NOT WIN32 OR NOT BUILD_SHARED_LIBS) + llama_build_and_test(test-grammar-integration.cpp) + llama_build_and_test(test-llama-grammar.cpp) + llama_build_and_test(test-batch-alloc.cpp) ++ llama_build_and_test(test-model-split.cpp) + llama_build_and_test(test-chat.cpp WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}) + target_include_directories(test-chat PRIVATE ${PROJECT_SOURCE_DIR}/tools/server) + target_link_libraries(test-chat PRIVATE server-context) +diff --git a/tests/test-model-split.cpp b/tests/test-model-split.cpp +new file mode 100644 +index 000000000..35cc84666 +--- /dev/null ++++ b/tests/test-model-split.cpp +@@ -0,0 +1,131 @@ ++#include "testing.h" ++ ++#include "llama.h" ++ ++#include "../src/llama-model.h" ++ ++#include ++#include ++#include ++#include ++ ++// The lookup load_tensors() performs for every offloaded layer. A well-formed set of split points ++// must map every layer in [0, n_layers) to a valid device index; that invariant is what these ++// tests pin, because breaking it is what turns a model load into a std::out_of_range. ++static bool all_layers_map_into_range(testing & t, const std::vector & splits, int n_layers) { ++ for (int il = 0; il < n_layers; ++il) { ++ int idx = -1; ++ try { ++ idx = llama_model_splits_select_device(splits, il, n_layers); ++ } catch (const std::exception & e) { ++ t.assert_true(std::string("layer ") + std::to_string(il) + " threw: " + e.what(), false); ++ return false; ++ } ++ if (idx < 0 || (size_t) idx >= splits.size()) { ++ t.assert_true(std::string("layer ") + std::to_string(il) + " out of range", false); ++ return false; ++ } ++ } ++ return true; ++} ++ ++static void test_normalize_proportional(testing & t) { ++ std::vector splits = { 1.0f, 3.0f }; ++ llama_model_splits_normalize(splits); ++ ++ t.assert_true("first split point is 1/4", std::fabs(splits[0] - 0.25f) < 1e-6f); ++ t.assert_true("last split point is 1", std::fabs(splits[1] - 1.00f) < 1e-6f); ++ t.assert_true("all layers map into range", all_layers_map_into_range(t, splits, 32)); ++} ++ ++static void test_normalize_single_device(testing & t) { ++ std::vector splits = { 42.0f }; ++ llama_model_splits_normalize(splits); ++ ++ t.assert_true("the only split point is 1", std::fabs(splits[0] - 1.0f) < 1e-6f); ++ t.assert_true("all layers map into range", all_layers_map_into_range(t, splits, 32)); ++} ++ ++// The regression this file exists for. A device that reports zero free memory -- e.g. a Metal ++// device whose currentAllocatedSize has grown past its recommendedMaxWorkingSetSize -- makes the ++// sum of the weights zero. Normalizing by that sum used to divide by zero, putting NaN in every ++// split point; NaN compares false against everything, so std::upper_bound returned the end ++// iterator and load_tensors() indexed one past the last device. The resulting std::out_of_range ++// surfaced as "error loading model: vector", which names nothing. ++static void test_normalize_zero_sum(testing & t) { ++ for (size_t n_devices : { (size_t) 1, (size_t) 2, (size_t) 4 }) { ++ std::vector splits(n_devices, 0.0f); ++ llama_model_splits_normalize(splits); ++ ++ bool finite = true; ++ for (size_t i = 0; i < splits.size(); ++i) { ++ finite = finite && std::isfinite(splits[i]); ++ } ++ ++ t.assert_true("no NaN split point", finite); ++ t.assert_true("split points are even", std::fabs(splits.back() - 1.0f) < 1e-6f); ++ t.assert_true("all layers map into range", all_layers_map_into_range(t, splits, 32)); ++ } ++} ++ ++// The same degenerate sum, reached from the other direction: --tensor-split values are parsed with ++// std::stof and never range-checked, so `-ts 1,-1` cancels out and lands here on any backend, with ++// no GPU memory pressure involved at all. ++static void test_normalize_cancelling_tensor_split(testing & t) { ++ std::vector splits = { 1.0f, -1.0f }; ++ llama_model_splits_normalize(splits); ++ ++ bool finite = true; ++ for (size_t i = 0; i < splits.size(); ++i) { ++ finite = finite && std::isfinite(splits[i]); ++ } ++ ++ t.assert_true("no NaN or inf split point", finite); ++ t.assert_true("all layers map into range", all_layers_map_into_range(t, splits, 32)); ++} ++ ++// The diagnostic half: a malformed set of split points must name itself rather than reaching the ++// device list and coming back out as libc++'s bare "vector". ++static void test_select_device_reports_malformed_splits(testing & t) { ++ const std::vector nan_splits = { std::nanf(""), std::nanf("") }; ++ ++ bool threw = false; ++ std::string what; ++ try { ++ llama_model_splits_select_device(nan_splits, 0, 32); ++ } catch (const std::exception & e) { ++ threw = true; ++ what = e.what(); ++ } ++ ++ t.assert_true("malformed split points throw", threw); ++ t.assert_true("message names the function", what.find("llama_model_splits_select_device") != std::string::npos); ++ t.assert_true("message names the layer", what.find("layer 0 of 32") != std::string::npos); ++ t.assert_true("message names the device index", what.find("device index 2") != std::string::npos); ++ t.assert_true("message prints the split points", what.find("nan") != std::string::npos); ++ t.assert_true("message is not just \"vector\"", what != "vector"); ++} ++ ++int main(int argc, char ** argv) { ++ testing t; ++ ++ const char * verbose = getenv("LLAMA_TEST_VERBOSE"); ++ if (verbose) { ++ t.verbose = std::string(verbose) == "1"; ++ } ++ if (!t.verbose) { ++ llama_log_set([](ggml_log_level, const char *, void *) {}, nullptr); ++ } ++ ++ if (argc > 1) { ++ t.set_filter(argv[1]); ++ } ++ ++ t.test("normalize_proportional", test_normalize_proportional); ++ t.test("normalize_single_device", test_normalize_single_device); ++ t.test("normalize_zero_sum", test_normalize_zero_sum); ++ t.test("normalize_cancelling_ts", test_normalize_cancelling_tensor_split); ++ t.test("select_device_malformed", test_select_device_reports_malformed_splits); ++ ++ return t.summary(); ++} diff --git a/llama/src/test/cpp/test_model_split.cpp b/llama/src/test/cpp/test_model_split.cpp new file mode 100644 index 00000000..5395af0d --- /dev/null +++ b/llama/src/test/cpp/test_model_split.cpp @@ -0,0 +1,114 @@ +// Runnable guard for patches/0012 (the layer-split fix in llama.cpp's src/llama-model.cpp). +// +// The patch also ships an upstream test (tests/test-model-split.cpp), but a FetchContent +// subproject builds with LLAMA_BUILD_TESTS=OFF, so that one is applied-but-never-compiled here. +// This file drives the same two functions from jllama_test, which every platform runs in CI -- +// so a llama.cpp bump that drops the patch reds "C++ Tests" everywhere instead of surfacing as +// one red macOS Java job with the message "error loading model: vector". +// +// Note that the failure it guards against is NOT reproducible on this side: it needs a GPU +// backend that reports zero free memory, and without one `devices` is empty and the mapping is +// never reached. What is testable -- and is what actually broke -- is the arithmetic itself. + +#include "llama-model.h" + +#include + +#include +#include +#include + +namespace { + +// The lookup load_tensors() performs for every offloaded layer. +void expect_every_layer_maps_into_range(const std::vector &splits, int n_layers) { + for (int il = 0; il < n_layers; ++il) { + int idx = -1; + ASSERT_NO_THROW(idx = llama_model_splits_select_device(splits, il, n_layers)) << "layer " << il; + EXPECT_GE(idx, 0) << "layer " << il; + EXPECT_LT((size_t) idx, splits.size()) << "layer " << il; + } +} + +} // namespace + +TEST(LlamaModelSplits, NormalizeIsProportionalToTheWeights) { + std::vector splits = {1.0f, 3.0f}; + llama_model_splits_normalize(splits); + + EXPECT_NEAR(splits[0], 0.25f, 1e-6f); + EXPECT_NEAR(splits[1], 1.00f, 1e-6f); +} + +TEST(LlamaModelSplits, NormalizeSingleDeviceTakesEverything) { + std::vector splits = {42.0f}; + llama_model_splits_normalize(splits); + + ASSERT_EQ(splits.size(), 1u); + EXPECT_NEAR(splits[0], 1.0f, 1e-6f); +} + +// The regression. A device reporting zero free memory -- e.g. a Metal device whose +// currentAllocatedSize has grown past its recommendedMaxWorkingSetSize -- makes the sum of the +// weights zero. Dividing by it put a NaN in every split point. +TEST(LlamaModelSplits, ZeroSumDoesNotProduceNaNSplitPoints) { + for (size_t n_devices : {(size_t) 1, (size_t) 2, (size_t) 4}) { + std::vector splits(n_devices, 0.0f); + llama_model_splits_normalize(splits); + + ASSERT_EQ(splits.size(), n_devices); + for (size_t i = 0; i < splits.size(); ++i) { + EXPECT_TRUE(std::isfinite(splits[i])) << "n_devices=" << n_devices << " i=" << i; + } + EXPECT_NEAR(splits.back(), 1.0f, 1e-6f) << "n_devices=" << n_devices; + } +} + +// NaN compares false against everything, so std::upper_bound returned the end iterator and +// load_tensors() indexed one past the last device -- an std::out_of_range whose libc++ what() is +// the bare string "vector". This is the assertion that would have failed before the fix. +TEST(LlamaModelSplits, ZeroSumStillMapsEveryLayerToARealDevice) { + for (size_t n_devices : {(size_t) 1, (size_t) 2, (size_t) 4}) { + std::vector splits(n_devices, 0.0f); + llama_model_splits_normalize(splits); + expect_every_layer_maps_into_range(splits, 32); + } +} + +// The same degenerate sum reached from the other direction, and the reason this is not a +// Metal-only defect: --tensor-split values are parsed with std::stof and never range-checked, so +// `-ts 1,-1` cancels out and lands on the very same line, on any backend, with no memory pressure. +// Unfixed this yields [inf, -nan] and every layer maps one past the last device. +TEST(LlamaModelSplits, CancellingTensorSplitStillMapsEveryLayerToARealDevice) { + std::vector splits = {1.0f, -1.0f}; + llama_model_splits_normalize(splits); + + for (size_t i = 0; i < splits.size(); ++i) { + EXPECT_TRUE(std::isfinite(splits[i])) << "i=" << i; + } + expect_every_layer_maps_into_range(splits, 32); +} + +TEST(LlamaModelSplits, ProportionalSplitsMapEveryLayerToARealDevice) { + std::vector splits = {1.0f, 3.0f}; + llama_model_splits_normalize(splits); + expect_every_layer_maps_into_range(splits, 32); +} + +// The diagnostic half: malformed split points must name themselves rather than reaching the +// device list and coming back out as libc++'s content-free "vector". +TEST(LlamaModelSplits, SelectDeviceNamesMalformedSplitPoints) { + const std::vector nan_splits = {std::nanf(""), std::nanf("")}; + + try { + llama_model_splits_select_device(nan_splits, 0, 32); + FAIL() << "expected malformed split points to throw"; + } catch (const std::exception &e) { + const std::string what = e.what(); + EXPECT_NE(what, "vector"); + EXPECT_NE(what.find("llama_model_splits_select_device"), std::string::npos) << what; + EXPECT_NE(what.find("layer 0 of 32"), std::string::npos) << what; + EXPECT_NE(what.find("device index 2"), std::string::npos) << what; + EXPECT_NE(what.find("nan"), std::string::npos) << what; + } +}