Skip to content

#59 MILTON_ATTN_MIN_TOKENS env override + report gate (tie fixture) - #61

Merged
tps-flint merged 3 commits into
mainfrom
cursor/attn-min-tokens-env-bf60
Sep 4, 2026
Merged

tps-flint merged 3 commits into
mainfrom
cursor/attn-min-tokens-env-bf60

Conversation

@heskew

@heskew heskew commented Sep 4, 2026

Copy link
Copy Markdown
Member

(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_TOKENS overrides 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.attnMinTokens is the effective gate after env/default (wasm getter after attnSetMinTokens). Compare receipt attn_crossover.gate uses that field. Unset env fail-closes if the report disagrees with compare-crossover.json gate: 32.
  • Flint rider: Rust default + effective getter asserted equal to the fixture gate; crossover test applies the export and asserts getter === extra.gate (32), plus MILTON_ATTN_MIN_TOKENS=64 override. lastThreadReport.attnMinTokens is on the load report.
  • Tiny wasm export (attnSetMinTokens / attnMinTokens). Remapped scripts/build-wasm.sh. CI run 33832387376 is 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 33832742129 SUCCESS (8/8), HEAD 36f4a0ebe99a18ad0d82bab2765541b3033f126c, base e6a5390fce9c3fce3f6d2b79e71a2ce17e845831.

npm test
# 123 JS pass; 62 rust + 3 metadata pass
# model::attn_gate_tests::default_gate_matches_crossover_literal ok
# tokenizer::unit::attn_crossover_fixtures_pin_min_tokens_boundary ok

npm run wasm:compare-verdict:threads
# GREEN n=21 failed=0 max_abs=0
# wasm_artifact=threads workers=4 kernel=relaxed
# thread_report.attnMinTokens=32
# attn-crossover-31/32/33 max_abs=0
# (re-run after swapping in CI blobs: still GREEN)

npm run wasm:compare-verdict
# GREEN n=21 failed=0 max_abs=0
# attnMinTokens=32

npm run wasm:must-fail
# named: layernorm fail 18 cases, pooling fail 18 cases, dropped-prefix fail 18 cases
# slipped: []

First CI (33832387376 on 05febb6): wasm RED on section-diff (bytes differ, no size delta). Committed that run's uploaded four blobs (artifact 9922152141). Second CI (33832742129 on 36f4a0e): 8/8 SUCCESS, including wasm byte-compare.

Override smoke (embed, lastThreadReport):

(default)              attnMinTokens=32
MILTON_ATTN_MIN_TOKENS=64  attnMinTokens=64
MILTON_ATTN_MIN_TOKENS=abc attnMinTokens=32  (warn once: out of range; using 32)

No epsilon / goldens / expected.json change.

Assignee

gh pr edit 61 --add-assignee heskew returned 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

Open in Web Open in Cursor 

cursoragent and others added 3 commits September 4, 2026 03:09
…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>
Crossover tests apply attnSetMinTokens on the relaxed glue directly so npm test does not require a GGUF. Report field after embed is covered by wasm-thread-report. Silence native dead_code on the wasm-only gate static.

Refs #59 #58

Co-authored-by: Nathan Heskew <heskew@users.noreply.github.com>
Local remapped rebuild differed in bytes with no section-size delta (same #50 lesson). Replaced all four artifacts with the wasm job upload. Glue unchanged. Threaded compare still workers=4 max_abs=0 attnMinTokens=32.

Refs #59

Co-authored-by: Nathan Heskew <heskew@users.noreply.github.com>

@tps-flint tps-flint left a comment

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.

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.

@heskew
heskew marked this pull request as ready for review September 4, 2026 03:29

@tps-sherlock tps-sherlock left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 "Number trims → 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 applyAttnMinTokensapi.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 tps-kern left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

APPROVED. All five disprove points on 36f4a0e:

  1. 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.

  2. 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.

  3. 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.

  4. 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").

  5. 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).

@tps-flint

Copy link
Copy Markdown
Collaborator

M4 pair on 36f4a0e vs main e6a5390 (quiet box, 3 rounds, CI blobs)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants