server: thinking-frontier bridge + vision encoder result cache

[nazerim]

Two independent server-side fixes from running the Vision-Exp model as a
coding-agent backend (pi.dev / OpenAI-compatible tool loops, 50k-300k
contexts). Both rebase and test cleanly on current main; neither depends
on image disk caching or on #927.

Provenance note: our fork (nazerim/ds4) carries @JordiPosthumus's #927
cherry-picked as commit 43ef837 (reuse KV when tools append images); it is
a prerequisite for our separate multimodal disk-cache work, which we plan
to submit once #927 merges. This PR is deliberately independent of #927 -
it rebases and tests cleanly on current main without it - so the two can
land in any order.

1. Bridge thinking turns in tool conversations to replay form.
   Plain (no tool-call) thinking turns in a tool-context conversation were
   excluded from the thinking-live checkpoint bridge by the has_tools gate.
   The live graph then diverges permanently from every faithful client
   replay at the think-token boundary (sampled open-think+reasoning vs
   replayed bytes, plus BPE re-merge differences across block boundaries),
   forcing full cold re-prefills on every later turn. Production symptom:
   repeated live kv cache misses with the common prefix stuck at the first
   thinking block (~30k tokens), drifting slightly right each turn. The fix
   remembers the exact bytes the next replay renders (prompt + sampled
   reasoning + close + visible + eos); the next request rebuilds from the
   exact live token prefix, immune to re-tokenization drift. Truncated
   (finish=length) turns still miss by design.

2. Vision encoder result cache.
   Every multimodal request re-encoded ALL history images through the ViT
   on every turn, even on live KV hits. Results are now cached by exact
   identity of the encoded bytes: entries carry the full encoded image, the
   128-bit hash is only a fast candidate filter, and a hit additionally
   requires memcmp over the complete bytes, so a collision is an ordinary
   miss, never a wrong-image embedding (LRU, fixed table). The cache is a ceiling, not
   a reservation: memory only materializes for embeddings actually produced
   and a session that never encodes images costs nothing; with 64 slots and
   a few-MiB vision embedding the table is the binding limit (worst case
   ~205 MiB), so the byte budget was set at 256 MiB - just above it as a
   safety net for larger embeddings (both constants are compile-time). Hits
   are copied out fresh, so request ownership semantics are unchanged.
   Authenticated-marker canonicalization is included (@JordiPosthumus's
   commit, adapted): the request-scoped image sentinel carries a random
   nonce, so a remembered visible transcript could never byte-prefix-match
   a future replay of the same multimodal history - the bridge was silently
   dead for image conversations. Instead of wildcarding marker-shaped text
   at match time (which would let literal marker-shaped user/tool content
   forge a frontier match when it later changed), the server canonicalizes
   the nonce bytes of a marker only after tokenization has located it via
   its known generated nonce; replay matching stays exact byte-prefix, so
   arbitrary marker-shaped text remains exact data. A guard still refuses
   the bridge when the rebuilt text suffix would carry new markers.

Review response (three rounds with @JordiPosthumus: stats race fixed,
lossy scope split to #971, budget right-sized to 256 MiB; his 8f003c3
exact-byte hit verification and 8945827 marker canonicalization are
cherry-picked with authorship preserved):

• Encoder-cache statistics race: the hits-before snapshot and the
  after-snapshots (hits, bytes) are all captured inside inference_mu - the
  same critical section as the cache traffic they observe - and passed as
  immutable values to the logger, which reads no shared state. Concurrent
  requests can no longer attribute each other's hits, and the delta can no
  longer underflow.
• Scope: auto-reduce of over-budget image histories moved out to server: opt-in auto-reduction of over-budget image histories (DS4_VISION_KEEP_IMAGES) #971 as
  its own opt-in PR (lossy API policy reviewed separately); the
  evidence-mismatch finding was a symptom of that mixing and is gone.
• Cache size rationale documented in code (see item 2).

Verification: ds4-server unit suite extended (preserved-canonical matches
future replay; authenticated-marker canonicalization matrix (literal marker-shaped text stays exact); embed cache store/hit/evict) plus
live tool+image conversations: cached 563/575 and 713/725 multimodal
continuations (previously full cold rebuilds), encoder reuse
encoder_runs=0 on replays. Those live numbers were gathered on our fork
build carrying the same bridge/cache code; the submitted head passes the
same unit suite on bare main.

[JordiPosthumus]

I audited the exact current head (65ad364) against current main (b0a147a) on an Apple M3 Ultra. The core ideas look worthwhile, especially preserving the thinking frontier through tool-context replays and avoiding repeated vision-encoder work.

Model-free validation was clean: default and CPU builds, server and agent tests, the remaining aggregate regression targets, ASan/UBSan server tests, and git diff --check. The aggregate make test reached the model-backed ds4_test but the clean worktree deliberately had no ds4flash.gguf, so I am not claiming a fresh real-model reproduction.

I found two concrete issues worth fixing before endorsement:

1. Encoder-cache statistics race. hits_before is read before inference_mu is acquired, then vembed_hits and vembed_bytes are read again after it is released. Request parsing can be concurrent, so one request can count another request's hits; count - (hits - hits_before) can consequently underflow. The cache data itself remains serialized, but these unsynchronized reads are a C data race. Capturing the before/after counters under the same critical section (or tracking per-request hits locally and passing immutable values to the logger) should make this small and clean.

2. Published evidence does not match this head. The submitted code has DS4_VISION_KEEP_IMAGES 16, but the PR's live validation says a 130-image request kept 128 and omitted 2. At this head it would keep 16 and omit 114. Please either rerun that boundary on the submitted configuration or align the implementation and description.

One scope concern: automatically discarding older images is an intentionally lossy API policy, whereas the thinking bridge and encoder cache are cache/correctness mechanisms. I think the image-retention policy would be easier to review as a separate PR, ideally explicit/opt-in. The fixed 512 MiB process cache is modest on very large hosts but may also deserve a short default rationale or configurability for smaller machines.

The preserved-thinking construction and image-marker wildcard otherwise look conservatively gated: exact visible bytes, matching live frontier/vision state, and refusal when a new marker appears in the suffix. I would be happy to rerun the same matrix after an update. Thanks for putting real production traces behind this work.

[nazerim]

Thanks for the careful audit — all three points were right. Update:

1. Stats race fixed in the revised cache commit: hits_before, the after
   delta, and the bytes snapshot are all taken inside inference_mu (the same
   critical section as the cache traffic), and ds4_vembed_log_reuse now
   takes only immutable values and reads no shared state. ds4_vembed_hit_counter
   is gone.

2. Evidence mismatch resolved by removing its cause: auto-reduce is no
   longer in this PR, so nothing here makes claims about a keep-count the
   submitted head doesn't have.

3. Scope split done: the lossy policy moved to server: opt-in auto-reduction of over-budget image histories (DS4_VISION_KEEP_IMAGES) #971 as its own PR, now
   explicitly opt-in via DS4_VISION_KEEP_IMAGES=N (unset = exact
   upstream behavior, requests over 16 rejected, error names the opt-in).
   The 512 MiB budget got its rationale comment (ceiling, not reservation;
   only materializes for embeddings actually produced).

This PR is now two commits: the thinking-frontier bridge and the encoder
result cache + marker-wildcard matching. Happy to have your matrix rerun on
the new head 75c92a6.

[nazerim]

Follow-up on my previous comment — the head moved twice since (now e5bcf15,
still two commits) and your small-host point turned out to deserve more than
a rationale comment:

• Cache budget right-sized. Measured embedding cost is ~3.2 MiB/image
  (196 tokens x 4096 hidden x fp32), so at 64 slots the slot table is always
  the binding limit (worst case ~205 MiB) and the 512 MiB byte cap was
  unreachable — a misleading number more than a memory risk. Budget lowered
  to 256 MiB, just above the slot ceiling, as a safety net for larger future
  embeddings. The comment now states which of the two limits binds.

• server: opt-in auto-reduction of over-budget image histories (DS4_VISION_KEEP_IMAGES) #971 keep-boundary evidence now rerun on the submitted configuration
  (was the earlier evidence-mismatch finding): live server built from
  server: opt-in auto-reduction of over-budget image histories (DS4_VISION_KEEP_IMAGES) #971's code with DS4_VISION_KEEP_IMAGES=16 — 20-image request served,
  log "kept last 16, omitted 4 oldest". Also verified on that build: unset
  default rejects 17 images with the opt-in hint in the 400 body, 2-image
  requests unaffected, and an invalid env value logs once and retains the
  reject policy. server: opt-in auto-reduction of over-budget image histories (DS4_VISION_KEEP_IMAGES) #971's description now carries these numbers.

Unit suite green on bare main for both branches. Matrix rerun welcome
whenever convenient.

[JordiPosthumus]

I re-audited the exact revised head e5bcf15. The earlier stats race, lossy image-retention scope, and budget-rationale findings are resolved. Clean default and CPU builds, optimized server/agent suites, ASan+UBSan server tests, and git diff --check all pass on Apple M3 Ultra. The sanitizer run covers instrumented C linked with the ordinary Metal object; it is not a GPU sanitizer or real-model run.

One correctness gap remains in the vision embedding cache. The current lookup treats (custom 128-bit hash, encoded_len) as exact image identity. A collision is unlikely accidentally, but it is still a false hit that returns the other image embedding; the current comment that a miss is the worst possible outcome is not true. This cache is process-wide, so exact identity is the cleaner invariant.

I prepared one focused commit directly on e5bcf15:

• 8f003c3 stores the encoded bytes inside the existing 256 MiB LRU budget and requires an exact memcmp after the hash/length filter.
• The byte budget now counts both the embedding and exact key.
• Entry cleanup owns both allocations in one helper.
• A forced same-hash, same-length, different-bytes regression proves that the candidate is rejected rather than reused.

The hash remains the fast filter, so misses do not gain an extra full comparison; only a matching candidate receives the exact check. Please feel free to cherry-pick or adapt the commit.

Packaging suggestion, not a correctness blocker: the two runtime ideas are independently useful. The easiest review shape may be one PR for the thinking-frontier bridge plus image-marker wildcard, and another for the exact-keyed encoder cache. If they remain together, moving the marker wildcard into the bridge commit would at least make each commit independently coherent.

No production source, model, cache, launcher, or running service was changed for this review. A fresh real Vision/Pi run should follow only after the submitted head settles.

[nazerim]

Cherry-picked your 8f003c3 (authorship preserved) — new head b49c173. Verified on it: default + optimized builds clean, full ds4-server unit suite green incl. the forced-collision regression, ASan+UBSan run of the server tests clean (macOS: heap/UB only, LeakSanitizer unsupported). The cache commit message was also reworded so it now honestly covers both hunks it carries (encoder cache + marker wildcards), addressing the independently-coherent point at message granularity. If you or @antirez still prefer the full 2-PR split (bridge+wildcard / exact-keyed cache), say the word and I'll cut it — held off to avoid a third history rewrite before the matrix rerun.

Also ported to our fork (same commit, 128-slot/512 MiB variant of the constants) and our production server is now running the exact-bytes-verified build.

[JordiPosthumus]

I found one more boundary while preparing the GB10 real-model comparison of b49c173: the generic marker wildcard also accepts literal marker-shaped text, not just parser-created image sentinels.

A concrete model-free reproduction on our production-shaped port has zero images, a remembered visible prefix containing \x1eDS4_IMAGE_aaaaaaaaaaaaaaaaaaaaaaaa\x1f, and a replay that changes those literal bytes to bbbb... and appends a new turn. The exact token prefix diverges, but the thinking tier still selects the old frontier. The image-identity gate passes because both requests have no images. The same underlying helper is present on the submitted head. Literal markers in user/tool content therefore need to remain exact data.

I prepared a smaller alternative to the wildcard in commit 8945827:

• Keep random nonces while rendering and locating the actual image inputs.
• After request_tokenize_multimodal_prompt() finds a marker by its known generated nonce and appends its vision tokens, normalize only that marker's nonce bytes in the internal rendered replay key.
• Ordinary byte-prefix matching then suffices. Real image markers are stable across replays; arbitrary marker-shaped text is never normalized or wildcarded.
• Image spans/fingerprints, embeddings, tokenization, the new-image suffix guard, and disk payloads are unchanged. There is no additional per-request/per-slot state.

This removes the generic wildcard scanner. Focused tests cover changed literals without images, a literal beside a genuine normalized marker, stable replay, shifted/shortened prefixes, and idempotence. Default build/server tests, CPU build, ASan+UBSan server tests and git diff --check pass on M3 Ultra. Sanitizers cover CPU C code, not GPU execution. The helper is based directly on your exact head; please cherry-pick or adapt if this shape suits you.

The GB10 model comparison is staged but has not run yet: its existing production request is still draining. No speed claim or production endorsement yet. I'll update this comment with the real-model evidence, including whether encoder hits preserve every embedding value and whether the thinking/image continuations retain their KV frontier.

[nazerim]

Cherry-picked 8945827 too (authorship preserved) — new head e3ca4b5. Your boundary is real and my original wildcard framing was wrong: shape-based matching at read time gives literal marker-shaped user/tool text a way to forge a frontier match; canonicalizing at the authenticated point (nonce generated by this server, located by strstr with that nonce, vision tokens already appended) keeps the invariant local and the matcher plain-exact. Good shape, thanks.

Verified on e3ca4b5: clean build, full unit suite green including your forced-literal-collision regression, ASan+UBSan clean. The PR description now describes the canonicalization design (item 2) and the verification section is corrected.

One port note for our fork variant: the marker-shape scanner stays there, since the disk-key normalizer uses it to substitute image fingerprints for nonces (nonce-independent either way, so disk keys remain compatible with snapshots written before this change). Only the thinking-tier matcher switched to exact bytes. Our production server runs e3ca4b5-equivalent code with reuse confirmed live (encoder_runs=0 on replays).

Standing by for the GB10 real-model evidence — and no rush; the current heads are stable. The 2-PR packaging offer (bridge+wildcard vs exact-keyed cache as separate PRs) remains open if your real-model pass wants them decoupled.