Skip to content

Never serve a masked decode from a staged speculation fire#420

Merged
shsym merged 1 commit into
pie.app/v1-base-shmemfrom
scratch/655-spec-mask-fix
Jun 16, 2026
Merged

Never serve a masked decode from a staged speculation fire#420
shsym merged 1 commit into
pie.app/v1-base-shmemfrom
scratch/655-spec-mask-fix

Conversation

@shsym

@shsym shsym commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Problem

Pass-level speculation pre-fires each context's next decode step before the inferlet has set up that step's constraints, so every staged fire is built with an empty constraint surface: no logit mask (build_next_request emits logit_masks = []) and no custom attention mask (masks = [], has_user_mask = false). The staged-hit check entry_matches_request compared anchor token/position and the spec arrays but neither mask — so a request carrying one was served the unconstrained, fully-causal staged token verbatim, silently dropping the constraint at the unconstrained→constrained transition.

Two reachable variants, both default-on (speculation_depth = 1):

  • Logit maskjson_object / json_schema / forced tool-call grammar decode ignored the grammar. A real model hides this (asked for JSON it naturally emits a leading {); the dummy driver (uniform-random within the mask) exposes it as fully unconstrained output that never begins with {.
  • Attention mask — single-token windowed-attention / attention-sink / hierarchical-attention decode (these decode one token at a time and call attention_mask() once the sequence exceeds the window) was served a token computed with full causal attention, so it attended to positions the inferlet meant to exclude.

Fix

Reject a staged hit whenever the incoming request carries either mask, forcing it onto the cold path where the mask is applied (the try_hit miss branch clears the now-stale chain). This mirrors the producer-side skip set in evaluate_request_shape, which already refuses to stage from either kind of masked request — the two sites must stay symmetric or the drop reappears.

A regression test per variant proves a masked request misses an otherwise-matching staged entry (both mutation-proven: each fails if its mask clause is reverted).

Also teaches the dummy driver to ack Copy/Adapter as no-ops (status 0) instead of the catch-all -1 error: a no-compute driver has no KV pages to copy and no adapter weights to load, and token generation is independent of cache contents, so a vacuous success is faithful. The previous -1 broke the context-fork path used for prefix sharing.

Provenance

Integration bug surfaced by the reconcile: this branch's runtime/src/inference/speculator.rs (pass-level speculation) and the pie-bridge logit_masks/masks schema were developed on separate upstream branches and were not wired together until the reconcile merged them onto one tree. Neither source branch exercised both halves, so the staged-fire-vs-masked-request interaction was never tested until now.

Verification

Tier Result
inference::speculator::tests (10) green — incl. entry_match_rejects_logit_masked_request + entry_match_rejects_user_masked_request, both mutation-proven
HTTP chat E2E 202 passed, 1 skipped (#200 pre-existing); json_object/json_schema output begins with {
Real Metal json_schema → 200 with {"name": ...}; forced tool → 200 with tool_calls; S3 coherence green
pie lib / weight-loader 444 / 14

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a51705a0-5835-43f4-a582-eaae29e51915

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch scratch/655-spec-mask-fix

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

… fire

The pass-level speculation chain extender pre-fires the next decode step
as soon as the previous one completes, before the inferlet has set up the
next step's constraints — so every staged fire is built with an EMPTY
constraint surface: no logit mask (`build_next_request` emits
`logit_masks = []`, since the grammar mask isn't known until the matcher
advances on the just-sampled token) and no custom attention mask
(`masks = []`, `has_user_mask = false`). The hit check
(`entry_matches_request`) compared anchor token/position and the spec
arrays but neither mask, so a request that carried one was served the
unconstrained, fully-causal staged token verbatim — the constraint was
silently dropped at the unconstrained→constrained transition.

Two reachable variants, both default-on (`speculation_depth = 1`):

  - Logit mask: a json_object / json_schema / forced-tool grammar decode
    whose anchor matched a staged entry ignored the grammar. A real model
    hides it (asked for JSON it naturally emits a leading `{`); the dummy
    driver (uniform-random within the mask) exposes it as fully
    unconstrained garbage that never begins with `{`.

  - Attention mask: a single-token decode supplying a custom attention
    mask (windowed-attention, attention-sink, hierarchical-attention
    inferlets, which decode one token at a time and call `attention_mask()`
    once the sequence exceeds the window) was served a token computed with
    full causal attention — the window mask silently dropped, so it
    attended to positions the inferlet meant to exclude.

Reject a staged hit whenever the incoming request carries either mask,
forcing it onto the cold path where the mask is actually applied (the
`try_hit` miss branch clears the now-stale chain). This mirrors the
producer-side skip set in `evaluate_request_shape`, which already refuses
to stage from either kind of masked request; the two sites must stay
symmetric. Adds regression tests for both variants proving a masked
request misses an otherwise-matching staged entry.

Also teach the dummy driver to ack Copy and Adapter requests as no-ops:
a no-compute driver has no KV pages to copy and no adapter weights to
load, and token generation is independent of cache contents, so the
faithful behavior is a vacuous success. Previously these hit the
catch-all arm and returned a driver error (-1), which broke the
context-fork path the runtime uses for prefix sharing.
@shsym
shsym force-pushed the scratch/655-spec-mask-fix branch from 6f21edf to 2fcece7 Compare June 16, 2026 14:53
@shsym shsym changed the title Never serve a logit-masked decode from a staged speculation fire Never serve a masked decode from a staged speculation fire Jun 16, 2026
@shsym
shsym merged commit 5ba0c43 into pie.app/v1-base-shmem Jun 16, 2026
7 checks passed
@shsym
shsym deleted the scratch/655-spec-mask-fix branch June 16, 2026 15:19
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.

1 participant