fix: P3 cleanup wave — legacy completions, alias listing, telemetry, terminal chunk, pool race, embedder gating#57
Merged
Conversation
…listing, telemetry, terminal chunk, pool race, embedder gating
- /v1/completions (+ POST /, /v1) accept legacy {"prompt"} bodies:
fallback decode wraps the prompt into a user turn (stream honored);
neither-shape bodies still 400 with the original decode error
- Ollama /api/tags reports the configured alias, matching /v1/models
(/api/show is a static stub with no id field — nothing to alias)
- Anthropic message_start.usage.input_tokens: documented-only — prompt
tokens only arrive on the terminal engine chunk, so deferring
message_start cannot help; message_delta carries the real counts
- Streaming telemetry counts real completion tokens (terminal
usage.completionTokens) instead of SSE/NDJSON frames, on all four
streaming paths, exactly once per request
- ModelPool: unload(id) during an in-flight load(id) is honored via a
tombstone — the completing load releases its byte reservation,
unloads the fresh engine, and skips insertion (no resurrection,
no reservation leak)
- EngineCoordinator residency-miss reconciles currentModel/status —
guarded by currentModelID == id so a stale-id generation can't
clobber a healthy newer model's status (review follow-up)
- MLXSwiftEngine.emitFinalChunk always emits one terminal chunk on a
non-cancel stream end (synthetic .stop + zero usage when the engine
closed without info) so reasoning splitters get their finish signal;
cancellation paths untouched
- /v1/embeddings + /v1/rerank return 400 model_not_embedder when the
resolved model isn't ModelFormat.embedder (404-when-missing kept)
7 new tests incl. a discriminating unload-during-load probe (budget
math proves the reservation released) and a K≠N token-counter test.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The P3 tail of the debug-round findings (follows #51 P0/P1 and #56 P2), plus the v0.5.2 embedder-gating follow-up. Eight small items:
/v1/completions— prompt-only bodies now work (fallback decode → user turn;streamhonored; neither-shape still 400)./api/tags— reports the configured alias, consistent with/v1/models. (/api/showis a static stub with no id field — verified nothing to alias.)message_start.usage.input_tokens— documented-only: prompt tokens only arrive on the engine's terminal chunk, so deferringmessage_startcan't help;message_deltaalready carries real counts.tokens_generated_totalnow accumulates the terminal chunk'scompletionTokens(was: SSE/NDJSON frame count), exactly once per request on all four streaming paths.unload(id)during an in-flightload(id)no longer resurrects the model: tombstone honored by the completing load (fresh engine unloaded, byte reservation released, insertion skipped).currentModel/statusinstead of leaving a stale.ready; guarded so a stale-id generation can't clobber a healthy newer model (review follow-up).emitFinalChunk— always emits one terminal chunk on non-cancel stream ends (synthetic.stop+ zero usage when no info), so reasoning splitters flush held-back partial tags; cancellation semantics untouched./v1/embeddings+/v1/rerankreturn 400model_not_embedderfor non-embedder targets (chat models no longer return meaningless vectors); missing models still 404.Review + verification
Adversarially reviewed (APPROVE; six targeted suspicions adjudicated — fallback-decode safety, tombstone lifecycle incl. no-await invariant + reservation exactly-once, terminal-chunk double-emit, telemetry exactly-once on all paths, gate placement, stale-id race; the one MEDIUM finding — status write outside the stale-id guard — is fixed here). Independently verified under both CI-equivalent conditions: xcodebuild
** TEST SUCCEEDED **(80 XCTest + 210 swift-testing, 0 failures; 7 new tests + all prior regressions + 11 DeepSeek parity) and bareswift test(210 passed, 0MLX error, gated tests skip cleanly); app build** BUILD SUCCEEDED **.