Skip to content

server: reject partial media truncation - #24076

Merged
ngxson merged 2 commits into
ggml-org:masterfrom
he-yufeng:fix/server-tokens-keep-media-boundary
Oct 5, 2026
Merged

ngxson merged 2 commits into
ggml-org:masterfrom
he-yufeng:fix/server-tokens-keep-media-boundary

Conversation

@he-yufeng

@he-yufeng he-yufeng commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • make server_tokens::keep_first validate the cut position itself when both sides are media tokens
  • reject truncation inside a media chunk instead of keeping the first media token and dropping the rest
  • add regression coverage for server_tokens using the mtmd test chunks
  • make the mtmd test image chunk copy-safe, since server_tokens copies media chunks internally

Fixes #24075.

Edit: Fixes #29866

Tests

  • git diff --check
  • cmake -S . -B build-codex -G Ninja -DLLAMA_BUILD_TESTS=ON -DLLAMA_BUILD_SERVER=ON -DLLAMA_BUILD_EXAMPLES=OFF -DLLAMA_BUILD_TOOLS=ON -DLLAMA_BUILD_APP=OFF -DLLAMA_BUILD_UI=OFF -DLLAMA_CURL=OFF -DCMAKE_C_FLAGS="-D_WIN32_WINNT=0x0A00" -DCMAKE_CXX_FLAGS="-D_WIN32_WINNT=0x0A00"
  • cmake --build build-codex --target test-server-tokens test-mtmd-c-api -j 2
  • cmake -E env "PATH=C:\mingw64\bin;$env:PATH" ctest --test-dir build-codex -R "test-server-tokens|test-mtmd-c-api" --output-on-failure

@he-yufeng
he-yufeng requested review from a team and ggerganov as code owners June 3, 2026 14:16
@github-actions github-actions Bot added testing Everything test related examples server labels Jun 3, 2026
@ngxson

ngxson commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

can you provide an example request (maybe as a python script) that can fully reproduce the issue?

Comment thread tests/test-server-tokens.cpp Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this, we don't do server test this way

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the current branch. The tests/test-server-tokens.cpp coverage was removed, and the request-level regression now lives in tools/server/tests/unit/test_vision_api.py following the existing server test structure.

Current checks only show labeler passing; I do not see a fresh failing server-test run on the latest head yet.

@he-yufeng
he-yufeng force-pushed the fix/server-tokens-keep-media-boundary branch from bba9d3e to ce99f9e Compare June 3, 2026 15:31
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Thanks, removed tests/test-server-tokens.cpp and the CMake entry in ce99f9e1.

A request-level repro needs a multimodal server and a context shift where n_keep lands inside the media token span. One way to trigger that is to run the server with a small context and send a multimodal /completion request with cache_prompt enabled:

import base64
import requests

base = "http://127.0.0.1:8080"
props = requests.get(f"{base}/props", timeout=10).json()
marker = props["media_marker"]

# 1x1 PNG. Any valid image works; this just keeps the repro small.
img = (
    "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk"
    "+A8AAQUBAScY42YAAAAASUVORK5CYII="
)

prompt = {
    "prompt": "alpha beta gamma delta epsilon " + marker + " zeta eta theta iota kappa lambda mu",
    "multimodal_data": [img],
}

payload = {
    "prompt": prompt,
    "cache_prompt": True,
    "n_keep": 6,
    "n_discard": 1,
    "n_predict": 64,
    "temperature": 0,
}

# With a small enough --ctx-size, this forces the shift path. The exact n_keep
# can be adjusted by +/- a few tokens depending on the tokenizer/template; the
# failing condition is that n_keep is between the first and last token of the
# media chunk.
for _ in range(2):
    r = requests.post(f"{base}/completion", json=payload, timeout=120)
    print(r.status_code, r.text[:500])

The old path checked find_chunk(n - 1), which accepts cuts after the first media token. This patch checks the cut position itself instead, so partial media truncation is rejected at the boundary instead of leaving the slot with a split media chunk.

@ngxson

ngxson commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

can you add one single test case to test_vision_api.py ? (please follow the same testing structure)

@he-yufeng
he-yufeng force-pushed the fix/server-tokens-keep-media-boundary branch from ce99f9e to 00c3547 Compare June 4, 2026 20:11
@github-actions github-actions Bot added the python python script changes label Jun 4, 2026
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Added the requested request-level coverage in tools/server/tests/unit/test_vision_api.py and pushed it in 00c3547b.

The new test sends a multimodal /completion request with n_keep=6, matching the first cut inside the media token span from the original boundary case, and asserts that the request is rejected with the media-boundary error instead of accepting a split media chunk.

Local validation:

python -m py_compile tools/server/tests/unit/test_vision_api.py
# passed

git diff --check
# passed

I also tried to run the focused pytest locally, but this Windows MinGW build is currently blocked before producing llama-server:

cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release
cmake --build build --target llama-server -j 4

The first build attempt needed _WIN32_WINNT=0x0A00 for cpp-httplib; after reconfiguring with that macro, the build progressed but failed in the UI asset embedding step (llama-ui-embed exits/hangs with 0xc0000139). Disabling local UI build/prebuilt UI still routes through the same embed helper. I kept the PR change scoped to the requested test and did not touch build/UI code.

@ngxson

ngxson commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

the test on CI didn't pass

@he-yufeng
he-yufeng force-pushed the fix/server-tokens-keep-media-boundary branch from 00c3547 to 97cef89 Compare June 6, 2026 17:08
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Updated in 97cef893.

The CI failure was caused by the added request-level test hitting context-size validation before it reached the media-boundary path. I changed the test to keep the request below the context limit, prime the slot cache with a successful multimodal completion, then repeat the same cached prompt so the cache trim lands inside the media chunk and returns Chunk not found.

Local validation:

python -m py_compile tools/server/tests/unit/test_vision_api.py
# passed

git diff --check
# passed

I also retried the focused pytest locally after installing the server test requirements. It is still blocked by this Windows environment: rebuilding build-codex fails in llama-ui-embed with 0xc0000139, and the existing build-codex binary did not reach /health within 60 seconds. I did not widen the PR to local Windows build tooling.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

I rechecked the current head (97cef893). The current pull_request workflows are stopped at action_required, so the server test has not rerun on this commit yet.

For the earlier failing run on 00c3547b, I checked the failed jobs and did not find a test_vision_api.py failure in the logs. The failures I found were unrelated self-hosted/environment jobs: CUDA test-backend-ops, Metal model download/network, and the KleidiAI rerank-tiny model conversion setup. The request-level test itself is still limited to tools/server/tests/unit/test_vision_api.py as requested.

So I am leaving the code unchanged for now; this branch needs the workflows approved/rerun before there is a fresh signal on the current test.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rechecked the current state: the only requested-change thread (removing the old C++ server test) is now outdated, because the branch moved the coverage into tools/server/tests/unit/test_vision_api.py as requested.

The current blocker appears to be action_required workflows rather than a fresh test failure on the latest head. Could you approve/rerun the workflows when you have a chance?

@ngxson

ngxson commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

sorry for the delay, I think this now needs a rebase

@he-yufeng
he-yufeng force-pushed the fix/server-tokens-keep-media-boundary branch from 97cef89 to 953acde Compare June 25, 2026 10:59
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (was 302 commits behind), pushed as 953acde. No conflicts — the three touched files (tools/mtmd/mtmd.cpp, tools/server/server-common.cpp, the test_vision_api.py regression) replayed cleanly.

The earlier review point (dropping the old C++ test-server-tokens.cpp in favour of a request-level test under tools/server/tests/unit/) is still in place. Should be ready for the workflows to run on a fresh head now.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Re-verified on current master: the keep_first path still resolves through find_chunk(n-1), so the selection bug this PR fixes is still there. Another round of review would help.

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Hi, I tried the new test locally: tinygemma3 uses SWA so the server reprocesses the whole prompt (n_past = 0) and keep_first never cuts, the second request returns 200 with and without the fix so the assertion fails, the fix itself looks right though.

ngxson
ngxson previously approved these changes Oct 4, 2026
@ngxson

ngxson commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

not sure why the CI doesn't trigger

@ngxson
ngxson marked this pull request as draft October 4, 2026 22:48
@ngxson
ngxson marked this pull request as ready for review October 4, 2026 22:48
@ngxson ngxson closed this Oct 4, 2026
@ngxson ngxson reopened this Oct 4, 2026
@github-actions github-actions Bot added the mtmd Related to multimodal functionality (video/image/audio) label Oct 4, 2026
@ngxson

ngxson commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

merging this when the CI passes

he-yufeng and others added 2 commits October 5, 2026 06:46
Drop the mtmd test helper change, which no longer builds since
clip_image_f32_batch stores its entries by value, and drop the
vision test: no test fixture reaches a cut between two adjacent
media chunks with a reused cache (tinygemma3 uses SWA and wraps
images in text tokens, tinyopenjev and small-test are recurrent),
so the test passed or failed independently of the fix.
@ServeurpersoCom
ServeurpersoCom force-pushed the fix/server-tokens-keep-media-boundary branch from 953acde to ca800b0 Compare October 5, 2026 04:48
@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Rebased on master and reduced to the keep_first one-liner: the mtmd.cpp hunk no longer built since entries are stored by value, and the vision test was dropped because no current fixture can reach a cut between two adjacent media chunks with a reused cache.

This PR fixes #29866

@ngxson
ngxson merged commit 8e16421 into ggml-org:master Oct 5, 2026
12 checks passed
edwardyoon pushed a commit to edwardyoon/focus-llama that referenced this pull request Oct 8, 2026
* server: reject partial media truncation

* server: keep only the keep_first fix

Drop the mtmd test helper change, which no longer builds since
clip_image_f32_batch stores its entries by value, and drop the
vision test: no test fixture reaches a cut between two adjacent
media chunks with a reused cache (tinygemma3 uses SWA and wraps
images in text tokens, tinyopenjev and small-test are recurrent),
so the test passed or failed independently of the fix.

---------

Co-authored-by: Pascal <admin@serveurperso.com>
(cherry picked from commit 8e16421)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

examples mtmd Related to multimodal functionality (video/image/audio) python python script changes server testing Everything test related

Projects

None yet

3 participants