# round 39 opus review — v0.1.45 serve/tokenizer/ple ports + astra qsa patch

read-only review. no source edits, gpu runs or service actions by me. scope: prompt cache, logit_bias cache,
telemetry fallback, ple dedup, astra qsa patch. paths are relative to `vendor/strata/` unless noted.

## what i compared

- `/tmp/v0145_serve_ple.patch` applied to a clean `git archive HEAD` copy (`/tmp/r39_audit_tree`, exit 0). the patch
  does **not** apply to the dirty tree (`serve/server.py:3852` conflicts with the local `STRATA_OMIT_IMAGES_WHEN_NO_VISION`
  block); that full `git apply` failed atomically, so nothing in it landed. the serve/tokenizer parts went in
  through the serve owner (working-tree server.py = the clean-applied copy plus the local omission block). PLE went
  in as **only upstream commit 36381540** (`results/round39/ple.patch`: ngram.cpp + ple_reader_test.cpp, no mutex).
  the later upstream commit that adds the `std::recursive_mutex` was **not imported**; the mutex was never in our
  tree, so nothing was dropped or reverted.
- `/tmp/astra_r39_qsa/astra_r39_qsa.patch` against current `metal/`.

## verdict

no remaining correctness blocker once the logit_bias `_last` removal (already done by the serve owner) is in.
three should-fix items before benchmark numbers or a persona eval are attributed to this build: telemetry
cost on darwin (S1), answer_room default (S2), prompt-cache cap (S3).

## should-fix

### S1. telemetry runs ioreg and creates a Metal device in the server every second (darwin)

- `serve/server.py:6485` `serve()` always calls `start_telemetry()`; on darwin `gpu_reader` now returns `_Metal`
  (`serve/telemetry.py` `gpu_reader`), so the sampler thread runs `ioreg -l -w0 -r -c IOAccelerator` every second
  (measured here on the M5: 39 ms wall, 57 KB of output; the patch says 22 ms) and
  `_Metal.read()` calls `metal_working_set()` on **every sample**, which does `MTLCreateSystemDefaultDevice()`
  (create rule: +1 retain that is never released) and the objc lookups each time. before the port, darwin had no
  gpu reader, so this is new work in the serving process during decode.
- fix: compute the working set once (`_Metal.__init__`) and reuse it in `read()` and in `Telemetry.static`.
- test: patch `telemetry.metal_working_set` with a counter, build `_Metal(run=fixture)`, call `read()` 3x, assert
  1 call. before citing any tok/s from this build: paired decode A/B with the sampler on vs `gpu_reader` forced to
  `_Nvml` (same engine, same prompts).
- semantics (not a blocker; it only matters for opt-in `min_free_vram_mib` / image policy): on this M5 at idle
  `"In use system memory"=15007744` (15 MB) vs `"Alloc system memory"=15473033216` (15.5 GB). free =
  working set − in-use probably reports far more free than there is while the wired arena is up. needs one check
  with the engine loaded before anyone turns on `min_free_vram_mib` on the mac.

### S2. `answer_room` is on by default and changes every thinking request on our config

- `serve/server.py:4206-4214`: with no `reasoning_budget_tokens`, every thinking request with `max_new >= 1024`
  gets budget = max_new − max(512, max_new/4). our config has `fit_max_tokens: true` and 32K context, so a request
  without max_tokens gets max_new = room and a budget of 75% of it, plus a `print` on **every** request
  (`max_new <= 32768` is always true at 32K ctx).
- when it fires, `REASONING_WRAP_UP` (`"I have thought about this long enough; time to give my answer."`) is injected
  into the reasoning stream, and the generation splits into a second engine pass. `frontend.py` additionally turns
  Anthropic `thinking.budget_tokens` into a hard budget.
- effort `low` keeps this rare, but persona/antislop evals before and after the port are not comparable unless
  this is pinned. decide explicitly: `"answer_room": false` in `models/K512_iq2_xs/strata-k512-iq2xs.json` for
  parity runs, or accept and note it. the per-request print should go behind a once-flag either way.
- model-card sampling (`model_card_sampling`): checked: a no-op for our config, because the `sampling` block sets
  temperature, top_p, top_k and presence_penalty, which covers every key in both qwen4exp sets.

### S3. PromptEncoder `keep` has no upper bound

- `serve/server.py` Service init: `keep = conversation_slots(engine)` is taken from `--conversation-cache-slots` with no clamp;
  the engine accepts up to INT32_MAX (`src/program/generate.cpp:2113`). each entry holds the full prompt str
  (1–4 B/char: kaomoji/CJK push the str to UCS-2/4), its ids list (8 B/token) and its mark lists, so ~2–5 MB at 100K
  tokens. our config passes no slots, so keep = 4 and this is latent.
- fix: `keep=min(slots, 16)` (or a total-chars budget). test: `conversation_slots` stub returning 10_000 → assert
  `svc.prompts.keep <= 16`; feed 20 unrelated prompts → `len(entries) <= keep`.

## checked and fine

### prompt cache exactness / mutation / concurrency (`tools/strata_tokenizer.py` PromptEncoder)

- the margin argument holds for `_encode_matching`: a match decision at q depends only on `text[q:q+max_special_len]`
  and on whether q is in a plain span; the finditer state before a cut c (end of a taken match) is identical when
  `c + max_special_len - 1 <= same`. `call_end`-style lookbehind does not exist in the literal alternation.
- fuzz (`/tmp/r39_fuzz.py`, `/tmp/r39_fuzz2.py`, real Swift tokenizer from `models/K256_reap_l2_sum/tokenizer`, 33
  specials, max len 21): conversations with appends, mid-inserts, truncations, specials split across appends
  (`<|im_sta` + `rt|>`), random plain spans, `store=False` mixed in, keep 1/2/4. 38,033 cases, 8,529 of them reusing
  a prefix, **0 mismatches** vs `tok.encode(text, parse_special=True, plain=...)`.
- mutation: `encode(store=True)` returns `list(ids)`, `store=False` returns a list that is not stored, entries are
  replaced and never changed in place; `prepare` never mutates `ids` in place (images build `out`, force builds a new
  list), so `forget_prompt(encoded_ids)` matches. the local omission block runs before `encode_prompt`
  (`serve/test_image_omission.py` covers it).
- concurrency: snapshot under the lock, BPE outside it, replace under the lock against the current list: a race
  loses reuse at worst, never data.
- `pick_slot` `TURN_ID = 248045` matches `<|im_start|>` in our vocab.
- parser `_wrapped_call_end` fast path: sound, because `call_end` only ever returns positions of a literal
  `</tool_call>`, and `_call_end_checked` is reset on every path that leaves the `call` state.

### logit_bias (`serve/logit_bias.py`)

- `_last` identity+len staleness: found by the parent, already removed by the serve owner.
- content key `json.dumps`: no collisions that change the result: `{1: x}` and `{"1": x}` serialize alike and
  normalize alike; float/bool keys, NaN and inf never normalize, so they are never stored; `False` vs `0` and
  `1` vs `"1"` serialize differently. results are copied (`normalize` → `dict(...)`), and the engine key is an
  immutable str. the lazy `entry[1]` write is a benign race (same string).
- cost note only: without `_last`, a 65K-entry list pays `json.dumps` on each of the ~5 calls per request.
  compute the key once in the route if that shows up in profiles.
- note: the Metal engine does not report `logit_bias=1`, so `validate_request` refuses non-empty biases on our
  backend anyway.

### PLE dedup (`src/kernels/ngram.cpp` `gather_batch`)

- any-slot matching plus `matched[t]` reuse is correct (keys are row ids; the table is immutable). dropping
  `n_prefetch >= n_tokens` is correct now that duplicates map to one slot. draining every ticket before returning
  an error fixes the old early return, which left tickets in flight writing into slots that would be reused.
- Metal uses Direct mode (`metal/src/strata_window_cli.cpp:945`), so this path is live: MTP `prefetch_rows`
  (`metal/src/engine/window.cpp:2972`) feeds the verify window's `gather_batch` (T = spec+1 ≤ 16). prompt chunks (2048)
  take the slow path, which drops the prefetches; that is fine.
- the later upstream `recursive_mutex` commit was not imported (only 36381540 was transplanted), so the table's
  threading is unchanged from our baseline. on Metal that baseline is fine: `ple_th` (`window.cpp:3434`) is joined
  inside the forward, and the MTP prefetch runs after it, so they never overlap. other backends (the desktop HIP build
  shares this file; upstream's mutex comment is about prompt-path / decode-window overlap there) carry the same
  unchanged baseline hazard and are untested in this round. record it in the port notes as "not imported". even with the mutex, `issue()` copies into `impl_->rows` before
  its `pending` check (pre-existing), so a second `issue()` while one is in flight corrupts the first ticket's keys:
  move the `pending` check first if this ever becomes multi-threaded.

### astra qsa patch

- `qsa_merge2` is dispatched only by `metal/tests/token_chain.cpp:249` and `metal/tests/qsa_parity.cpp:49,159,254`.
  the engine dispatches `qsa_merge` (v1) at `metal/src/engine/window.cpp:1285,2737,3662` and
  `metal/src/engine/prompt_chain.inc:262`. `qsa2.metal` is still compiled into `lqsa_` (`window.cpp:763`), so the change is
  compile-surface only: **zero runtime effect, no end-to-end attribution**. the unroll keeps the fma accumulation order
  (it only hoists loads), which matches astra's bitwise result.
- `qsa_prompt.metal` clamped-row edits are in the `!coop_k4` branches only; on the M5, `Window::coop_k4()` probes true,
  so they run only with `COOP_K4=0` or on M1/M4. it is a safety port, with no M5 default-path effect. row 0 / scale 0 for
  empty cells is in bounds whenever the cache holds ≥1 cell.
- `moe_mm_parity` `TK=64` / `v2||v3`: test-only. the baseline default was already broken (missing
  `mm_gu_42_16_64_16_2`, exit 3).

## tests i ran (cpu only, `PYTHONDONTWRITEBYTECODE=1`, from `vendor/strata`)

- `python -m unittest serve.test_prompt_encoder serve.test_logit_bias serve.test_image_omission serve.test_model_card_sampling serve.test_admit_order`: 33 tests OK
  (before the serve owner's `_last` fix landed).
- `python -m unittest serve.test_server.MacTelemetry serve.test_server.AmdTelemetry`: 7 OK.
- `python tools/test_strata_tokenizer.py`: 11 OK, 3 skipped.
- PromptEncoder fuzz above: 0 mismatches.
- not run: anything on the gpu, ple_reader_test (the parent ran it), the full serve suite.

## followup v2 — sdk ownership check for S1

- `MacOSX.sdk/System/Library/Frameworks/Metal.framework/Headers/MTLDevice.h` (CommandLineTools sdk on this machine):
  `MTL_EXTERN id <MTLDevice> __nullable MTLCreateSystemDefaultDevice(void) API_AVAILABLE(macos(10.11), ios(8.0)) NS_RETURNS_RETAINED;`
  so the caller owns +1, and each uncached `metal_working_set()` call leaked one retain on the device singleton. that
  is a refcount, not memory per call, and the cost is the per-sample framework call. with the working set cached
  once, a single unreleased +1 at startup is acceptable. an optional `objc_msgSend(dev, sel_registerName("release"))`
  after reading would make it exact, but it is not needed.
- the same header says a call switches a dual-gpu intel mac to the high-power gpu. it does not apply to apple
  silicon, but it is one more reason to call it once and not every second.
- answer_room: parent approved keeping the live default; it stays disclosed in S2, with no comparable eval numbers
  claimed.

## followup v3 — hardening review (final sources)

- the fingerprints in `results/round39/integration-source-sha256.json` match the working tree for all 27 files.
- telemetry: `_Metal.__init__` reads the working set once (`self.working_set`), and both `read()` and `Telemetry.static`
  reuse it. `metal_working_set()` releases the device in a `finally` (`objc_release`, matching the
  `NS_RETURNS_RETAINED` annotation); the early `if not dev` return happens before the try, so a null is never released. covered by
  `test_working_set_is_read_once_for_samples_and_static` plus the release assertion. what remains per sample: the 1 Hz `ioreg`
  subprocess (39 ms wall measured earlier); its cost on decode is still not A/B measured. `Service.free_vram_mib()` still
  builds a fresh reader per call (2 ioreg runs + 1 working-set read), but only on the opt-in and image paths.
- prompt cache: `PROMPT_ENCODER_KEEP_MAX = 16` caps slots from args and from INFO, and the default stays 4. covered by
  `IncrementalPrompts` (asserts 10**9 → 16 on both paths). `tools/strata_tokenizer.py` is byte-identical to the
  fuzzed version, so the 38,033-case result still applies to the final source.
- logit_bias: `_last` is fully removed and only the bounded JSON-keyed cache remains; nothing else changed.
- reran on the final sources (cpu): test_prompt_encoder, test_logit_bias, test_image_omission, MacTelemetry, AmdTelemetry:
  33 OK; IncrementalPrompts: 9 OK.
- verdict: S1 (per-sample device creation) and S3 (cap) are resolved. S2 is accepted and disclosed by the parent.

## followup v4 — provenance of `results/round39/model-ab/` (read-only; no processes launched)

- `ngram-baseline.cpp` is byte-identical to `git show HEAD:src/kernels/ngram.cpp` (ngram.cpp was clean before the
  round). against the current file the diff is 15+/11-, i.e. the 36381540 dedup only.
- host archive: 34 members, same names and order as `build-metal/libstrata_metal_host.a`. extracted and hashed: only
  `ngram.cpp.o` and the ranlib `__.SYMDEF` index differ, and the swapped member equals `model-ab/ngram.cpp.o`. no other
  linked archive defines `PleTable::gather_batch` (`libstrata_metal_kernels.a` only references it), so link order
  cannot pick a stale copy. the disassembly of `PleTable::gather_batch` differs between `baseline-cli` (340 lines)
  and `candidate-cli` (391 lines).
- `candidate-cli` is byte-identical to `build-metal/strata_window_cli`, built at 11:59:22, after the last round39
  source write (qsa files 11:59:21, ngram.cpp 11:56:32).
- shaders: the runtime directory comes from `STRATA_METAL_KERNELS` (`metal/src/mtl_rt.mm:51`), with no on-disk library
  cache in mtl_rt. `candidate-kernels/` equals the current `metal/kernels/` file for file (22/22).
  `baseline-kernels/` differs from it only in `qsa2.metal` and `qsa_prompt.metal`, matches `/tmp/astra_r39_qsa/baseline`
  exactly, and matches the sha256 values in astra's `metadata.json` for qsa/qsa2/qsa_prompt.
  the engine-private msl (`mtp.metal`, `mtp_chain.metal`, `ple.metal`, `slop4.h`) loads from the compiled-in absolute
  path for both arms; all four were last written before the round (02:57–08:11, and slop4.h on 10-17), so this is shared and
  unchanged.
- consequence for interpreting results: on the M5 the shader delta is inert (`qsa_merge2` is not dispatched, and the
  non-coop branches are not taken), so the arms differ in effect only by the PLE gather_batch dedup in the MTP verify path.
- suggestions, not blockers: (1) both arms should emit identical token ids, because the dedup returns the same bytes;
  compare the generated ids per pair as a correctness gate. (2) the 10K case decodes ~31 tokens in ~0.9 s, which is too
  short a window to resolve a 2% decode delta with 3 pairs; call that case's decode inconclusive unless the spread is
  clearly under 2%, and read prefill separately.

## followup v5 — reading the paired CLI A/B (logs only: `results/round39/model-ab/runs.json`, `summary.json`)

amended at the parent's request: everything below is descriptive. nothing here is timed beyond what runs.json
records, and no statistical bound is claimed.

- output token IDs equal in all 5 measured pairs (and the warm pair). aggregate `STRATA_SPEC` rounds/drafted/accepted are equal
  within each case for every run of both arms (10K: 13/23/18; 2K: 39/99/88). equal aggregates support comparable work;
  they do not establish an identical acceptance trajectory, which is not logged and is not inferred from the output IDs.
- drift: decode ms/token rises overall through the session, 28.65 → 33.05 over the 10K series, but not monotonically
  (10K decode 920.70 ms, then 915.71 ms in consecutive runs; 2K 4213.72 then 4143.13). it stays at 31.5–33.2 in the
  2K series. 10K prefill goes 21.6 s → 30.4 s. this affects both arms. sustained-load throttling on a fanless Air is
  plausible but unmeasured (no thermal readings), so no cause is claimed.
- descriptive drift-adjusted fit (OLS: value ~ run index + arm, computed from runs.json; n = 6 and n = 4, so no
  confidence intervals are claimed):
  - 10K decode: −5.9 ms (−0.6%), residuals up to ±22 ms.
  - 10K prefill: −431 ms (−1.7%), residuals up to ±1.0 s.
  - 2K decode: +64.6 ms (+1.6%), residuals ±49 ms, 1 dof.
  - 2K prefill: +210 ms (+3.2%), residuals ±232 ms, 1 dof.
- prefill as a same-path comparison: prompt chunks call `gather_batch` with T = 2048 (one 1984-token chunk for 2K;
  4×2048 + 1988 for 10K). old and new code take the same slow path for T > 16, and the shader changes are not
  executed on the M5 default path. the prefill arm differences nonetheless have opposite signs (−1.7%, +3.2%). this
  shows the run-to-run instability of this setup; it is not a statistically established noise floor.
- code-path note (an operation count, kept separate from the measurements; it does not enter the verdict): in decode, the verify windows call `gather_batch` with T ≤ 4. the new
  matching adds at most 4×16 key comparisons of 64 B each and issues no more SSD read tickets than the old code (its
  fast path covers every case the old fast path did; the non-fast path is unchanged). its latency was not timed.
- verdict: inconclusive for small performance changes. no consistent slowdown is visible in 10K; the 2K decode
  differences (+1.5–1.7% per pair) cannot be separated from the drift and instability in this run. a cooled, stationary
  experiment would be needed to resolve differences at this scale. the parent's wording in `audits/r39_integration.md`
  matches this.