fix: reject LLM calls on a context that cannot serve them - #35
Merged
Conversation
llm_chat_cursor_open() allocates the ai_cursor before validating that the
connection has a usable context, then returns SQLITE_ERROR without freeing it.
sqlite does not call xClose for a cursor whose xOpen failed, so the allocation
is lost for the lifetime of the connection - 48 bytes per rejected statement.
Reachable today by querying the vtab with no context created:
SELECT llm_model_load('model.gguf');
SELECT reply FROM llm_chat('hi'); -- errors, and leaks
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
…sinks
ai->context and ai->vtab are the error sink for the whole chat and sampler
subsystem. Roughly twenty sites report through
sqlite_common_set_error(ai->context, ai->vtab, ...) - llm_chat_run(),
llm_chat_generate_response(), llm_chat_tokenize_input(), llm_chat_save_response(),
llm_chat_check_context(), llm_sampler_check() - and none of them receives the
caller directly. Only three places ever assigned those fields, so at nearly every
report site the sink was whatever the previous statement happened to leave behind.
Two consequences, both reachable from ordinary SQL:
SELECT llm_context_create_chat('context_size=512');
SELECT llm_chat_respond('hi'); -- publishes this statement's context
SELECT llm_context_free();
SELECT llm_chat_restore('x'); -- reports into the finalized statement
That last line called sqlite3_result_error() on freed memory: SIGSEGV. Without
the llm_chat_respond() line the sink is still NULL and the same call returned
NULL with no error at all. After a vtab scan the sink is the vtab instead, so
the message went into vtab->zErrMsg, which sqlite only imports immediately after
a vtab method - silently discarded, and the sqlite3_vmprintf buffer leaked.
llm_context_create_with_options() had the same bug from the other direction: it
reported a llama_init_from_model() failure through ai->context/ai->vtab while
every other error in that function used the context parameter it was handed. On
a fresh connection both fields are NULL and the failure vanished entirely, so
the function returned NULL as though it had succeeded.
The rule now: every entry point publishes its own sink before running anything
that can fail. Scalars own a sqlite3_context, vtab methods own the sqlite3_vtab.
Applied at all of them:
- chat scalars: create, free, save, restore, system_prompt, respond
- vtab: xConnect, xOpen, xFilter, xNext, xClose. xFilter and xNext matter as
much as xOpen - both reach llm_chat_run() and llm_chat_generate_response(),
and an interleaved scalar statement can repoint the sink mid-scan.
- the 16 llm_sampler_* scalars, which reach llm_sampler_check(). Its
allocation failure is the one report site with no caller of its own, so
without this an OOM right after a vtab scan wrote into a vtab->zErrMsg that
sqlite no longer imports: message lost, buffer leaked.
llm_chat_disconnect() additionally left ai->vtab dangling at freed memory after
the vtab was released, which is what turned the second case above from a lost
message into a use-after-free. The ai_context outlives the vtab, so the
back-pointer has to be cleared.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
Closes #33. llm_text_generate() only checked that a context existed, so calling it against an embedding context ran on incompatible state. Pooled embeddings leave no per-token logits, so the first sampled token reads as EOG, the loop breaks immediately, and the empty buffer is returned as '' with no error. The chat paths are worse: llm_chat_respond() and the llm_chat() vtab reach llama_sampler_sample(), which dereferences that buffer. Track what the active llama context can actually do and reject the operations that need per-token logits with SQLITE_MISUSE. The kind is derived from how the context is configured, not from which llm_context_create_* wrapper was called, because two different things make a context an embedding context: - generate_embedding=1 was passed. llm_context_create() is a general-purpose constructor and API.md documents llm_context_create('generate_embedding=1,normalize_embedding=1,pooling_type=mean') as equivalent to llm_context_create_embedding(), so classifying by call site would leave #33 unfixed on the documented generic form. - the model pools by default. A BERT-family model (all-MiniLM, nomic-embed) carries its pooling type in its own GGUF, so llm_context_create('context_size=512,embedding_type=FLOAT32') on one yields a pooling context with no embedding settings passed at all - and that is the flow API.md recommends for embeddings, so #33 reproduced there too. Resolved pooling alone cannot decide this: forcing pooling_type=mean on a generative model still generates correctly. What identifies an embedding model is pooling the caller did not ask for, which llama makes visible by resolving LLAMA_POOLING_TYPE_UNSPECIFIED from the model's own hparams. So the test is ctx_params.embeddings, or an unrequested resolved pooling type. That makes "did the caller ask for pooling" load-bearing, which exposed a bug in the option parser: generate_embedding forced pooling_type = MEAN for any value, 0 included. An explicit generate_embedding=0 therefore both configured a pooling context while asking for embeddings to be off, and marked pooling as caller-requested - hiding embedding models from the check. The side effects now apply only when embeddings are actually enabled. Note llama_model_has_decoder() is no use here - it returns true for everything except T5ENCODER, BERT included. Text generation and chat contexts share one kind: llm_context_create_chat() and llm_context_create_textgen() both pass the same empty option string, so the contexts are byte-for-byte identical and must stay interchangeable - the README vision example creates a chat context and then calls llm_text_generate(). Embedding generation is deliberately NOT gated. What it needs is a context that pools, and llm_embed_generate_run() already checks the resolved pooling type - a better test than the declared kind, and one that keeps working on every configuration above. In the vtab the check is skipped when no context exists at all, so a missing context still reports the actionable "No context found" rather than a kind mismatch against LLM_CONTEXT_NONE. Known limitation, documented in API.md: passing pooling_type explicitly on an embedding model opts out of the second test and lets generation return '' again. That is the cost of not misreading a caller-forced pooling type on a generative model, which is the more common case. Tests cover both directions on both model families: the rejections, and that a generic-constructor embedding context, a caller-forced pooling type, and a chat context all keep working. Covering the embedding-model half needs an encoder-style model, so the suite now pulls all-MiniLM-L6-v2 (25MB) alongside the existing generative model; that test skips without --embed-model. CI never reaches the Makefile's download rule for the other models: the download-models job fetches them on the host and every build job restores them from cache. The new model is wired through that same path - URL hash, job output, host-side restore/download/verify, and a cache restore plus directory in the build jobs. Without it `make test` fell through to curl, which the Alpine container used by the linux-musl arm64 jobs does not have (Error 127). Bumps SQLITE_AI_VERSION to 1.0.8. The workflow reads it via `make version` and tags from it, so the bump is what makes the release job publish rather than warn that the version matches the latest release. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
andinux
force-pushed
the
fix/context-kind-from-options
branch
from
August 31, 2026 09:05
3e1d6ff to
06554a2
Compare
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.
Closes #33.
Supersedes #34, which diagnosed the bug correctly but classified contexts by which
llm_context_create_*wrapper was called. That left #33 unfixed on the documented generic form and broke working configurations. This takes the same idea — record what the active context is for, reject operations that cannot work on it — and derives the classification from how the context is actually configured.Three commits, each independently revertable.
ba55a1c— release the cursor whenllm_chat()xOpen failsllm_chat_cursor_open()allocated theai_cursorbefore validating the context and returnedSQLITE_ERRORwithout freeing it. SQLite does not call xClose for a cursor whose xOpen failed, so every rejectedSELECT ... FROM llm_chat(...)leaked 48 bytes. Pre-existing; reachable today by querying the vtab with no context created.46e68be— report failures to the caller being served, not to stale sinksai->context/ai->vtabare the error sink for the whole chat and sampler subsystem. About twenty sites report through them; only three ever assigned them, so at nearly every report site the sink was whatever the previous statement left behind.That last line called
sqlite3_result_error()on freed memory — SIGSEGV. Without thellm_chat_respond()line the sink is NULL and the same call returned NULL with no error at all. After a vtab scan the message went intovtab->zErrMsg, which SQLite no longer imports: discarded, and the buffer leaked.Every entry point now publishes its own sink first: the chat scalars, the vtab methods (xFilter and xNext matter as much as xOpen — an interleaved scalar statement can repoint the sink mid-scan), and the 16
llm_sampler_*scalars that reachllm_sampler_check().llm_chat_disconnect()also leftai->vtabdangling at freed memory, which is what made the second case a use-after-free rather than a lost message.Pre-existing and unrelated to #33, bundled here because the gate is unreportable without it.
22d3086— reject generation on a context that cannot produce tokensPooled embeddings leave no per-token logits, so the first sampled token reads as EOG and generation returns
''with no error. The chat paths are worse:llm_chat_respond()and thellm_chat()vtab reachllama_sampler_sample(), which dereferences that buffer.A context is an embedding context when either:
generate_embedding=1was passed — API.md documentsllm_context_create('generate_embedding=1,...')as equivalent tollm_context_create_embedding(), so classifying by call site leaves llm_text_generate() returns '' with no error when the active model/context is an embedding one #33 unfixed on the documented form; orllm_context_create('context_size=512,embedding_type=FLOAT32')onall-MiniLMyields a pooling context with no embedding settings passed at all — and that is the flow API.md recommends for embeddings, so llm_text_generate() returns '' with no error when the active model/context is an embedding one #33 reproduced there too.Resolved pooling alone cannot decide this: forcing
pooling_type=meanon a generative model still generates correctly. What identifies an embedding model is pooling the caller did not ask for, which llama exposes by resolvingLLAMA_POOLING_TYPE_UNSPECIFIEDfrom the model's own hparams. (llama_model_has_decoder()is no use — it returns true for everything except T5ENCODER, BERT included.)That made "did the caller ask for pooling" load-bearing, which exposed a parser bug:
generate_embeddingforcedpooling_type = MEANfor any value,0included. The side effects now apply only when embeddings are actually enabled.Embedding generation is deliberately not gated — it needs a context that pools, and
llm_embed_generate_run()already checks the resolved pooling type, which is a better test than the declared kind.Known limitation, documented in API.md: passing
pooling_typeexplicitly on an embedding model opts out of the second test and lets generation return''again. That is the cost of not misreading a caller-forced pooling type on a generative model, which is the more common case.Compatibility
The gate fires only where generation was already broken — silent
''or a crash — so no working configuration starts failing. Verified across both model families: generic-constructor embedding contexts, caller-forced pooling, chat contexts used withllm_text_generate()(the README vision example), andgenerate_embedding=0on a generative model all keep working.Tests
56 passing, up from 51. Each new test was confirmed to fail against the pre-fix code before being accepted, including a 96-byte leak assertion for the cursor and a SIGSEGV reproduction for the error sink.
Covering the embedding-model half needs an encoder-style model, so the suite now pulls
all-MiniLM-L6-v2(25MB) alongside the existing generative model. That test skips cleanly without--embed-model, so existing checkouts are unaffected.Follow-ups, not addressed here
''rather than an error for anything the classifier does not catch, including thepooling_typeopt-out above. This is the general form of llm_text_generate() returns '' with no error when the active model/context is an embedding one #33 and has now surfaced twice in different guises.llm_chat_create()installs an unseededdistsampler onai->samplerthat outlives the context, silently making laterllm_text_generate()calls non-deterministic.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ