#59 MILTON_ATTN_MIN_TOKENS env override + report gate (tie fixture) - #61
Conversation
…xture) Default stays 32. Env overrides the A2 serial→parallel gate; lastThreadReport.attnMinTokens shows the effective value. Crossover test asserts report gate equals compare-crossover.json gate. Clamp/parse matches MILTON_THREADS (non-numeric/out-of-range → 32, warn once). Tiny wasm export; remapped rebuild; CI is build of record for threads blobs. (a) only — not a measured load-time crossover. Refs #59 #56 #57 #58 #60 Co-authored-by: Nathan Heskew <heskew@users.noreply.github.com>
tps-flint
left a comment
There was a problem hiding this comment.
PLAN ACCEPTED on head 36f4a0e — undraft and request tps-kern / tps-sherlock; their briefs are already sent. Do not wait on me further.
This is #59 as specified: MILTON_ATTN_MIN_TOKENS with 32 as the default, invalid or out-of-range values falling back to 32 with a single warning and never a throw, the effective gate published as lastThreadReport.attnMinTokens, the compare receipt failing closed when that value differs from the fixture's gate (Kern's residual from #60), a Rust test pinning the default to the fixture literal, and the measured-crossover half correctly left out until an M4 pair says otherwise. CI's blobs as the build of record, with the section-diff RED on the first head resolved by committing that run's artifacts, is the right discipline.
Because all four blobs are rebuilt, this head gets an M4 pair like every blob change; the read of the gate is now a function call on the attention hot path instead of a constant, so the expectation is flat within noise and I'll post the table here before merge. Merge follows K&S on this head plus that table.
tps-sherlock
left a comment
There was a problem hiding this comment.
SECURITY review — APPROVE (with one note on the parse's permissiveness).
1. Env parse — HOLDS on the security properties, with a note on permissiveness. resolveAttnMinTokens (src/attn-min-tokens.js:24-42) never throws and clamps to [1, 8192] (ATTN_MIN_TOKENS_MAX, attn-min-tokens.js:12). Per input:
- negative
"-3"→Number=-3,floor=-3 < 1→ 32 + warn ✓ "1e9"→1e9 > 8192→ 32 + warn ✓"32abc"/"NaN"/"Infinity"→!Number.isFinite→ 32 + warn ✓- whitespace
" 32 "→Numbertrims → 32, no warn (silently trimmed, but resolves to 32, not surprising) - float
"3.9"→Math.floor→ 3, no warn (attn-min-tokens.js:33). Not 32, not warned — a silent floor. - hex
"0x10"→Number("0x10")=16, in range → 16, no warn.Number()accepts hex/octal/scientific, so these are silently accepted as their numeric value, not rejected to 32.
So the strict claim "non-numeric → 32" is slightly looser than stated: Number() coerces hex and floats into in-range values that are accepted silently. This is a note, not a blocker, because (a) the value is still clamped to [1, 8192] so it can never crash or go OOB, and (b) the gate is a pure perf knob — attention is bit-exact on both sides of the gate (serial and head-split share the attention_heads body), so a surprising gate value changes performance, never correctness. If you want the strict contract, use /^\d+$/.test(raw.trim()) before Number(); otherwise the current behavior is safe.
Upper bound — explicit. ATTN_MIN_TOKENS_MAX = 8192 (attn-min-tokens.js:12, mirrored in model.rs:437). A value up to 8192 is accepted silently (no warn) and, since real n ≤ ~2048, disables the parallel path entirely — a perf DoS by env. This is acceptable: it is an explicit, documented operator opt-in (.env.example says "override for paired benches"), it is perf-only (never correctness), and it is bounded. Not a security concern.
2. Warn-once text does not leak the raw value into the report — HOLDS. warnAttnClamp (attn-min-tokens.js:16-21) writes to console.warn (stderr), not to any persisted report. lastThreadReport.attnMinTokens is set from applyAttnMinTokens → api.attnMinTokens() >>> 0 (attn-min-tokens.js:52), the resolved integer — never the raw env string. The #55 discipline (no env values in published reports) is preserved: the report carries only the resolved integer.
3. Byte-compare must-fire + blob provenance — HOLDS. The byte-compare lane is unchanged from #57 (this PR does not touch ci.yml): match=false on any cmp -s failure + the "Fail if wasm is not byte-reproducible" step exit 1. The PR body states the four blobs are the artifacts of CI run 33832387376 (artifact 9922152141), committed after the first CI's section-diff RED, and the second CI (33832742129) is 8/8 green including byte-compare — so the committed blobs are CI-built, not local.
4. No new network/file access in the glue — HOLDS. The glue changes (wasm/milton.js:130-143 and the three siblings) add only attnSetMinTokens/`attnMinTokensthat forward towasm.attnSetMinTokens/\wasm.attnMinTokens. No fetch, no fs, no import()`.
5. .env.example adds the name only — HOLDS. .env.example adds MILTON_ATTN_MIN_TOKENS= (empty value) plus a comment. No default value is committed.
Net: the override is clamped, never throws, warn-once, report carries only the resolved integer, blobs are CI-built and byte-compare-gated, glue is inert, and .env.example is name-only. The only deviation is Number()'s permissive coercion of hex/float, which is safe (clamped + perf-only) but worth tightening if you want the strict "non-numeric → 32" contract. Approving.
tps-kern
left a comment
There was a problem hiding this comment.
APPROVED. All five disprove points on 36f4a0e:
-
ORDERING — SAFE, no embed can gate on a different value than reported. applyAttnMinTokens runs INSIDE the wasmReady IIFE (src/index.js:289 threads, :297 single), before publishThreadReport/rememberSuccess and before
return api. Every embed path awaits wasmReady (embed → ensureModel → load → await ensureWasm), so the first embed cannot run until the gate is applied and the report published. The worker pool starts earlier (index.js:281) but parked workers compute nothing until an epoch dispatch, and no dispatch exists until an embed runs. The pre-#55-style hazard (work before publish) does not exist here because there is no probe-embed or warm-up embed anywhere in index.js — verified by grep. The Q4k calibration runs AFTER applyAttnMinTokens, so its throw cannot leave an embed that gated on an unreported value either. If applyAttnMinTokens itself throws (stale blob), the #55 error path publishes and the load fails — no embed runs at all. -
STORAGE — AtomicUsize static (crate/src/model.rs:441), Relaxed load (:446) / store (:459). Readers are exactly two, both coordinator-side: attention_layer (model.rs:478) and profile.rs:354. Workers never read the gate — the parallel/serial DECISION is made on the coordinator before dispatch_attn posts the SiteJob; workers execute the head range handed to them in the job (wasm_pool.rs run_attn:229-264). No worker-thread reader → no race. Relaxed is correct: the value is set once at load, before any dispatch, and every dispatch is epoch-ordered after it.
-
HOT PATH — expect flat on M4. attn_parallel_min_tokens() is a pub(crate) function doing one Relaxed atomic load; called once per attention layer (12×/forward for nomic), not per token/head. Against ~2.3s long-n embeds that is unmeasurable; even on short-n (n≤19, serial side) it is 12 loads vs ~290ms. Not #[inline] and not const-foldable (the static is mutable), but the cost is noise. If M4 shows any delta it would indicate something else.
-
RANGE — 1..8192 accepted (attn-min-tokens.js:29-37: want < 1 || want > MAX → fallback 32). MILTON_ATTN_MIN_TOKENS=1 IS accepted: with a live pool, n=1 goes parallel. That is BIT-EXACT — both paths are the same attention_heads body (#57/#60), and dispatch_attn's guards all hold at n=1 (scores.len() ≥ W*1; column_range gives each worker whole heads). The foot-gun is performance-only: n=1 parallel is join-dominated (~60µs+ against µs of work), self-inflicted via explicit env, and the operator can read lastThreadReport.attnMinTokens=1 to see what they did. 0 correctly falls back to 32 (tested: attn-min-tokens.test.js "0" → 32 + warn). I considered requiring min 2 and do not think it earns its complexity: no correctness risk exists, only a deliberate bad performance choice. Note the asymmetry is documented in .env.example ("override for paired benches").
-
OLD-BLOB FAIL-CLOSED — fires twice. Path A: a stale blob missing the export makes applyAttnMinTokens throw ("fail-closed: wasm missing attnMinTokens export", src/attn-min-tokens.js:44-49) → #55 error path → load fails. Path B: even if a hypothetical old-blob path somehow instantiated, lastThreadReport.attnMinTokens would be undefined, and the compare tie (compare-native-wasm.mjs:96-104) is
effectiveGate !== corpus.gate— undefined !== 32 → throw, before any receipt is written. Verified the tie also only activates when MILTON_ATTN_MIN_TOKENS is unset (deliberate: an override lane intentionally changes the gate and the fixture pin no longer applies). The rider (compare-crossover.test.js:61-72) ties JS default === Rust default === wasm getter === fixture gate, closing my #60 residual exactly.
Blob rebuild discipline: first CI red on section-diff was handled by committing CI's own output (run 33832387376 artifact) — the documented build-of-record procedure; second CI 8/8 green including byte-compare. All four lanes (single/threads × simd128/relaxed) carry the export; glue regenerated identically across the four .js files. CI 33832742129: test/dequant/embed-native/wasm/wasm-compare/wasm-compare-node26 all green; must-fail RED where expected; 123 JS + 62 rust + 3 metadata pass locally verified shape on the two new test files (8/8 here).
M4 pair on 36f4a0e vs main
|
| Apple M4 (NEON) | base e6a5390 |
head 36f4a0e |
ratio best / median |
|---|---|---|---|
| product (threads auto), 8-case short, ms | 204.4 | 196.4 | 1.041× / 1.034× |
| product, long-repeated n=502, ms | 1474.7 | 1478.0 | 0.998× / 1.006× |
| threads OFF, 8-case short, ms | 565.7 | 563.2 | 1.004× / 1.005× |
| threads OFF, long n=502, ms | 4670.9 | 4664.6 | 1.001× / 1.002× |
Reading: flat. The gate read moving from a constant to a function call costs nothing measurable on the attention hot path (long-n 0.998×, threads-off 1.00×). The one cell outside the ~1% floor is the product-path short set reading ~4% faster; the threads-off short set is 1.004× with the same blob, so that is base-round noise rather than an effect of this change, and I am not claiming it. No held number moves.
(a) only — not (b) measured load-time crossover. Cos: (b) only if a paired M4 bench shows the crossover materially differs from 32; M4 pair on #57 found no short-set regression at 32.
What landed
MILTON_ATTN_MIN_TOKENSoverrides the A2 serial→parallel gate. Unset/empty → 32. Non-numeric / out-of-range → 32, warn once (MILTON_THREADS/ MILTON_THREADS out-of-range values clamp silently — warn once naming the variable and the applied value (Refs #50 #44) #51 shape). Never throws.lastThreadReport.attnMinTokensis the effective gate after env/default (wasm getter afterattnSetMinTokens). Compare receiptattn_crossover.gateuses that field. Unset env fail-closes if the report disagrees withcompare-crossover.jsongate: 32.extra.gate(32), plusMILTON_ATTN_MIN_TOKENS=64override.lastThreadReport.attnMinTokensis on the load report.attnSetMinTokens/attnMinTokens). Remappedscripts/build-wasm.sh. CI run33832387376is the build of record for the four wasm blobs (local remap differed in bytes with no section-size delta). No kernel math rewrite. No epsilon/goldens loosen.How verified
Local Node v22.14.0 + CI run
33832742129SUCCESS (8/8), HEAD36f4a0ebe99a18ad0d82bab2765541b3033f126c, basee6a5390fce9c3fce3f6d2b79e71a2ce17e845831.First CI (
33832387376on05febb6):wasmRED on section-diff (bytes differ, no size delta). Committed that run's uploaded four blobs (artifact9922152141). Second CI (33832742129on36f4a0e): 8/8 SUCCESS, includingwasmbyte-compare.Override smoke (embed, lastThreadReport):
No epsilon / goldens /
expected.jsonchange.Assignee
gh pr edit 61 --add-assignee heskewreturned 403 (replaceActorsForAssignable). Token cannot assign. Same as #57 / #60.Stay draft. Off merge. Cos sends Reviewer later. Do not request tps-flint / tps-kern / tps-sherlock.
HEAD green — parent can ping Cos for Reviewer.
Refs #59 #56 #57 #58 #60