Fix duplicate token append in Metal generator - #306
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c40b70dd2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| printf("%.*s", (int) token_size, (const char*) token_ptr); | ||
|
|
||
| status = gptoss_context_append_tokens(context, 1, &predicted_token); | ||
| if (status != gptoss_status_success) { | ||
| fprintf(stderr, "Error: failed to append predicted token %" PRIu32 " to context\n", predicted_token); | ||
| goto error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop before sampling beyond the context buffer
When generation does not encounter the return token and --max-tokens is omitted or exceeds the remaining context capacity, removing the append also removes the only capacity check: after the last valid slot is filled, this loop calls gptoss_context_sample() again, which writes the sampled token at context->num_tokens without checking context->max_tokens, one element past token_buffer. The removed append previously returned gptoss_status_context_overflow as soon as the context became full; replace that accidental guard with an explicit remaining-capacity check before the next sample.
Useful? React with 👍 / 👎.
Metal
generate.cappends each sampled token twiceSummary
In the Metal backend,
gptoss_context_sample()already appends every sampled token to theContext. Thegenerateexample then callsgptoss_context_append_tokens()with the same token. Consequently, each token is printed once but is present twice in the internal sequence used to predict later tokens.This report applies to the current upstream
HEADverified on 2026-08-27:Relevant code
Inside
gptoss_context_sample(), the sampled token is written tocontext->token_buffer, after which the function advances the context:The generated tokens are then copied from that same internal token buffer to
tokens_out.After
gptoss_context_sample()returns,gpt_oss/metal/source/generate.cappendspredicted_tokenagain:Since
context->num_tokens == context->num_kv_tokensat this point,gptoss_context_append_tokens()takes its ordinary append path and writes a second copy of the token.Expected behavior
After generating and printing tokens
t1, t2, ..., the internal autoregressive context should be:Actual behavior
The internal context becomes:
while stdout still contains only:
The first generated token is unaffected, but later tokens are conditioned on the duplicated sequence. In longer continuations this can produce unnecessary repetition and confused self-corrections.
Minimal state-based reproduction
The duplication can be verified without interpreting generated text. Around the existing calls in
generate.c, recordcontext->num_tokens:The first assertion confirms that sampling already advanced the visible context. The second confirms that the example added the same token a second time.
Minimal fix
Remove the explicit
gptoss_context_append_tokens()call from the generation loop.gptoss_context_sample()already advances the context and supports generating multiple tokens in one call.The accompanying patch also clarifies the side effect in the public function comment, because the current wording does not state that sampling mutates the
Context.Verification
We kept the original executable for reproduction and built a second generator that differs only by omitting the extra append. The clean generator:
The issue was found while tracing how the first generated token enters the KV-cached incremental path used to predict the second token.