fix(skippy): make local proposal handoff deadline-safe - #1193
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLocal generation now tracks canonical tokens emitted between proposal boundaries. Linear proposal queries carry these tokens and request/session identities. The plugin host validates and commits token prefixes before proposal events and final receipt completion. ChangesPending token proposal flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LocalGeneration
participant LinearProposalQuery
participant ActivePlugin
participant Plugin
LocalGeneration->>LinearProposalQuery: submit pending canonical token IDs
LinearProposalQuery->>ActivePlugin: proposal query and request/session identities
ActivePlugin->>Plugin: commit pending tokens
ActivePlugin->>Plugin: request proposal lookup or report outcome
ActivePlugin->>Plugin: commit final receipt suffix
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/local_generation/tests.rs (1)
89-110: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the verified-proposal boundary.
The direct test calls
commit_local_generation_tokentwice. It does not exerciseemit_token. The integration test setslinear_proposal_ingresstoNoneandnative_mtp_enabledtofalse, so it covers ordinary decoding only.Add a controlled proposal-path test that checks lifecycle visibility before the next proposal lookup. Otherwise, rename the direct test to describe helper sequencing rather than proposal-boundary coverage.
Also applies to: 226-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/local_generation/tests.rs` around lines 89 - 110, The test local_generation_commits_advance_before_the_next_proposal_boundary currently validates only direct helper sequencing, not proposal-path lifecycle visibility. Add a controlled test exercising emit_token with a verified proposal and assert the commit is visible before the next proposal lookup; alternatively, rename the existing test to describe commit_local_generation_token sequencing and remove the proposal-boundary claim.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-server/src/frontend/local_generation/token_generation.rs`:
- Around line 45-50: The run_decode_loop token-emission path must ensure
GenerationReceiptConfig::committed delivery completes before the next
try_execute_linear_proposal lookup. Update the flow around emit_token and
GenerationReceiptConfig::committed to enforce synchronous ordering, or skip the
queued configuration for this changed path, while preserving commit contents and
ordering.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/local_generation/tests.rs`:
- Around line 89-110: The test
local_generation_commits_advance_before_the_next_proposal_boundary currently
validates only direct helper sequencing, not proposal-path lifecycle visibility.
Add a controlled test exercising emit_token with a verified proposal and assert
the commit is visible before the next proposal lookup; alternatively, rename the
existing test to describe commit_local_generation_token sequencing and remove
the proposal-boundary claim.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fbbcc4d5-b63d-4c3c-8eb1-9efb85fb9371
📒 Files selected for processing (2)
crates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-server/src/frontend/generation_receipt.rs`:
- Around line 241-256: Update flush and submit_ordered so ordered delivery
cannot wait indefinitely when the lifecycle worker or deliver_to stalls. Add a
bounded deadline or cancellation-aware receive/send path that returns an error
on timeout, allowing the caller at Line 340 to record a failed receipt while
preserving successful ordering behavior.
- Around line 206-212: Update the QueuedGenerationLifecycleMessage handling in
the generation worker so each Barrier reflects observation delivery failures
since the preceding barrier. In the Observation branch, record failures as
currently; in the Barrier branch, consume or reset that interval’s failure state
and send an appropriate Err(...) through completion when any occurred, otherwise
retain Ok(()). Ensure committed_before_proposal cannot proceed after a failed
GenerationCommit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d78fa543-6b7f-4ca5-b1d3-39dea5660a59
📒 Files selected for processing (3)
crates/skippy-server/src/frontend/generation_receipt.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/skippy-server/src/frontend/local_generation/token_generation.rs
- crates/skippy-server/src/frontend/local_generation/tests.rs
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
🤖 (micn's agent) Reviewed for 0.75.1. The diagnosis is convincing — the lifecycle refactor (4bd1453) gave remote generation commit delivery but left local generation updating only the final receipt, so lifecycle consumers hold a stale canonical position and the boundary check correctly fails closed on the next proposal. Emitting an authoritative commit per local token from the single emit_token closure is the right shape, and covering both ordinary decode and verified proposal spans through one emission point avoids the double-count trap. CI is green and the strengthened receipt test verifying the exact commit stream is good regression coverage.
Three things worth a look before this ships in a point release:
1. Inference can now fail on lifecycle delivery problems — previously best-effort. commit_local_generation_token propagates committed_before_proposal errors as OpenAiResult out of emit_token, so a stalled/failed lifecycle sink aborts user generation after the 250ms timeout. The pre-existing contract elsewhere in this file is explicitly the opposite (recording_failures: "These failures disable receipt recording for the affected generation but never fail inference"). Failing closed is arguably correct when a proposal consumer genuinely depends on the canonical position — a stale position would just fail the boundary check anyway — but when receipts are enabled with no proposal consumer (metrics/audit-only sinks), a slow plugin sink now kills generations that previously succeeded. Was that trade-off intended? If not, one option: only use the ordered path when the linear-proposal ingress is active, and fall back to best-effort committed() otherwise.
2. Per-token synchronous barrier on the hot decode path. Every emitted token now does: boxed-slice alloc → channel send (with yield_now spin when full) → rendezvous-channel barrier → wait for the worker to drain the queue. At local decode rates this serializes token emission against the delivery worker, so a sink that takes even ~1ms per commit caps generation at ~1k tok/s and any transient stall inserts visible latency. The function is named committed_before_proposal but it runs on every token whenever receipts are enabled, including when the proposal path is disabled (linear_proposal_enabled == false, native MTP, hooks active). Same conditional as (1) would fix both: pay the ordered cost only where ordering buys correctness.
3. Double-counted failure metric. One failed commit increments both the worker's delivery_failures and (via the barrier error → inspect_err) submission_failures, so delivery_failures() reports 2 for a single failure — the new queued_barrier_reports_delivery_failure_and_resets_interval test even asserts == 2. Anyone alerting on this counter will see inflated numbers. Minor, but worth either not incrementing submission_failures for barrier-reported failures, or documenting that the counter can count one failure twice.
Also a small naming nit: failures_since_barrier resets only on the error path — on the Ok path it's already 0 so behavior is fine, just slightly odd to read.
None of these block the correctness fix; (1) is the one I'd want a deliberate answer on before 0.75.1, since it changes user-visible failure semantics for existing receipt users.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
might want to consider breaking inference case
| fn wait_for_commits(sink: &RecordingReceiptSink, expected: usize) { | ||
| for _ in 0..100 { | ||
| if sink.commits.lock().unwrap().len() >= expected { | ||
| return; | ||
| } | ||
| thread::sleep(Duration::from_millis(1)); | ||
| } | ||
| panic!("timed out waiting for {expected} generation commits"); | ||
| } | ||
|
|
There was a problem hiding this comment.
@coderabbitai does rust have a stronger pattern for this instead of busy-wait looping?
There was a problem hiding this comment.
@ndizazzo Yes. Use std::sync::Condvar with the commit storage.
RecordingReceiptSink::committed can notify the condition variable after it pushes a commit. Then wait_for_commits can use Condvar::wait_timeout_while to block until the expected count arrives or a single timeout expires.
This avoids the 1 ms polling interval and reduces test scheduler sensitivity. A channel with recv_timeout is also valid, but Condvar fits the existing shared Vec<GenerationCommit> state best.
You are interacting with an AI system.
i386
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-native-serving-plugin-host/src/lib.rs (1)
378-405: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear tracking when final suffix delivery fails.
commit_final_suffixcan return before Line 405. The entry then remains incommitted_generated_tokens, andfinish_generationis not called. Remove the entry on everyfinishresult.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 378 - 405, Update finish so the committed_generated_tokens entry for the receipt key is removed on every exit, including when commit_final_suffix returns an error. Ensure cleanup occurs before propagating that error while preserving the existing finish_generation result handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 1379-1386: Update the test around the existing proposal commit and
raw callback calls to construct a LinearProposalReceipt and invoke
ActivePlugin::report via active.report(&receipt). Use this high-level method in
place of the direct report_proposal callback while preserving the expected event
ordering assertions.
- Around line 338-342: Reject lifecycle events for generations without a
successful start: at crates/mesh-native-serving-plugin-host/src/lib.rs:338-342,
require an existing committed_generated_tokens tracking entry instead of
deriving a fallback prior count; at
crates/mesh-native-serving-plugin-host/src/lib.rs:415-420, reject finish
processing when that tracking entry is absent so no suffix commit or finish
event is emitted.
---
Outside diff comments:
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 378-405: Update finish so the committed_generated_tokens entry for
the receipt key is removed on every exit, including when commit_final_suffix
returns an error. Ensure cleanup occurs before propagating that error while
preserving the existing finish_generation result handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc43fe68-5cc2-4bd8-b7d9-37afc948d51f
📒 Files selected for processing (7)
crates/mesh-native-serving-plugin-host/src/lib.rscrates/skippy-server/src/frontend/generation_receipt.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/linear_proposal/execution.rscrates/skippy-server/src/frontend/local_generation/linear_decode.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rs
| let prior_generated_token_count = self | ||
| .committed_generated_tokens | ||
| .get(&key) | ||
| .copied() | ||
| .unwrap_or_else(|| generated_token_count.saturating_sub(token_ids.len())); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject lifecycle events for generations that did not start.
A failed begin_generation does not create a tracking entry. The fallback accepts later commits, and finish can then send a suffix commit and finish event without a successful start.
crates/mesh-native-serving-plugin-host/src/lib.rs#L338-L342: Require an existing tracking entry instead of deriving a prior count from the incoming commit.crates/mesh-native-serving-plugin-host/src/lib.rs#L415-L420: Reject finish processing when no tracking entry exists.
📍 Affects 1 file
crates/mesh-native-serving-plugin-host/src/lib.rs#L338-L342(this comment)crates/mesh-native-serving-plugin-host/src/lib.rs#L415-L420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 338 - 342,
Reject lifecycle events for generations without a successful start: at
crates/mesh-native-serving-plugin-host/src/lib.rs:338-342, require an existing
committed_generated_tokens tracking entry instead of deriving a fallback prior
count; at crates/mesh-native-serving-plugin-host/src/lib.rs:415-420, reject
finish processing when that tracking entry is absent so no suffix commit or
finish event is emitted.
| active.commit_proposal_tokens(1, 2, &[4, 5]).unwrap(); | ||
| let status = unsafe { | ||
| (active.definition.api().report_proposal)(active.instance().unwrap(), std::ptr::null()) | ||
| }; | ||
| assert_eq!(status, abi::PluginStatus::OK); | ||
| active.commit_final_suffix(1, 2, &[4, 5]).unwrap(); | ||
|
|
||
| assert_eq!(*events.lock().unwrap(), ["begin", "commit", "report"]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call ActivePlugin::report in this test.
The test calls commit_proposal_tokens and the raw report_proposal callback. It does not test the ordering in ActivePlugin::report. Build a LinearProposalReceipt and call active.report(&receipt).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 1379 - 1386,
Update the test around the existing proposal commit and raw callback calls to
construct a LinearProposalReceipt and invoke ActivePlugin::report via
active.report(&receipt). Use this high-level method in place of the direct
report_proposal callback while preserving the expected event ordering
assertions.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
🤖 (micn's agent) Re-reviewed after 52b077ed ("keep proposal commits off lifecycle hot path"). This is a substantially better design than the barrier approach and resolves both of my earlier concerns at the root rather than patching around them:
- Hot path: the synchronous submit-and-flush barrier is gone entirely (
submit_ordered/flush/Barriermachinery deleted). The canonical token delta now travels on the proposal query itself (LinearProposalQuery::pending_token_ids), so the cost is only paid when a proposal is actually being made — a single boxed-slice clone on a path that's about to do a runtime decode anyway. Ordinary decode with no proposal consumer is back to fire-and-forget. This also kills the failure-semantics concern: lifecycle delivery can no longer abort inference. - Double-counted failure metric: gone with the barrier.
- The new
Skippedoutcome is a nice touch — when there's no proposal capacity the pending delta stays owned by the caller instead of being consumed-but-undelivered, andquery_does_not_accept_pending_tokens_without_proposal_capacitylocks that in. - The plugin-host side is careful: per-
(request_id, session_id)committed-count tracking, prefix-extension validation (commit does not extend the tracked generated-token prefix), cleanup on abort/finish, andcommit_final_suffixcovering the tail between the last proposal boundary and generation end — so a plugin still receives a complete, contiguous commit stream even for generations where the proposal path never fires.
Two small observations, neither blocking:
unwrap_or_else(|| generated_token_count.saturating_sub(token_ids.len()))incommit_tokens— for an unknown key this trusts the caller's count to back-derive the prior prefix. It's only reachable if a commit arrives without abegin(or after cleanup), where failing closed with abail!might be preferable to synthesizing a prefix. In practicebeginalways seeds the entry, so this is defensive-path behavior only.- Semantics shift worth one line in the PR description:
GenerationReceiptSink::committeddeliveries are now proposal-boundary-driven (plus final suffix) rather than strictly per-token-real-time for every configuration. The doc comment on the trait was updated to say exactly this ("optionally observes ... deltas"), which is honest — just make sure the PR body's "every token emitted" framing matches the final shape so 0.75.1 notes are accurate.
CI is green on the branch. Happy with this for the point release once the merge conflict with main (the #1180 proposal-deadline change touched the same files) is resolved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mesh-native-serving-plugin-host/src/lib.rs (2)
469-479: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop proposal lookup after the commit reaches the deadline.
commit_tokenscan block incommit_generation. If that callback reachesquery.deadline, this method still callsstart_proposal. The expired request can then run a new plugin callback and delay later FIFO work.After the pending-token commit, return
Ok(None)when the deadline has elapsed. Add a test that verifies the fakeproposalcallback is not called in this case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 469 - 479, After the pending-token `commit_tokens` call in the surrounding request-processing method, check whether `query.deadline` has elapsed and return `Ok(None)` before invoking `start_proposal`. Add coverage using the fake proposal callback to verify it is not called when the commit reaches or passes the deadline.
570-575: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize receipt token commits before later proposal queries.
PluginCommand::Reportruns on the passive worker. Itscommit_proposal_tokenscall can occur after the primary worker processes a laterProposalfor the same request.For example, if a receipt must commit token
Aand the next query carries tokenBat generated count2, the later query can see tracked count0. The contiguous-prefix check then rejectsBbecause it expects count1.Commit the receipt prefix in primary-queue order before dispatching
report_proposalon the passive worker. Add a delayed-report regression test with a later proposal query for the same request and session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 570 - 575, Serialize receipt token commits before passive-worker reporting by moving the commit performed by report (and its report_proposal dispatch path) into primary-queue order, ensuring later Proposal queries observe the committed prefix. Update the relevant PluginCommand::Report/report flow without changing token validation semantics, and add a regression test covering a delayed report followed by a later proposal query for the same request and session.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 469-479: After the pending-token `commit_tokens` call in the
surrounding request-processing method, check whether `query.deadline` has
elapsed and return `Ok(None)` before invoking `start_proposal`. Add coverage
using the fake proposal callback to verify it is not called when the commit
reaches or passes the deadline.
- Around line 570-575: Serialize receipt token commits before passive-worker
reporting by moving the commit performed by report (and its report_proposal
dispatch path) into primary-queue order, ensuring later Proposal queries observe
the committed prefix. Update the relevant PluginCommand::Report/report flow
without changing token validation semantics, and add a regression test covering
a delayed report followed by a later proposal query for the same request and
session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2cb0572-b869-420b-9072-a093f9acff10
📒 Files selected for processing (5)
crates/mesh-native-serving-plugin-host/src/lib.rscrates/mesh-native-serving-plugin-host/src/plugin_dispatch/tests.rscrates/mesh-native-serving-plugin-host/src/test_support.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/local_generation/linear_decode.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/skippy-server/src/frontend/local_generation/linear_decode.rs
3 participants
Problem
4bd1453fad1ab93ce14b010f76f91c368e929fb6moved canonical commit delivery into the lifecycle path. Local generation did not publish the same deltas, so after one locally decoded token the next proposal query could observe a stale consumer position and fail closed.Delivering every local token through an ordered lifecycle barrier fixes that position mismatch, but it also puts queue pressure and arbitrary lifecycle callback latency on inference. That is unsafe for the 8 ms proposal deadline: a slow or blocked consumer must not block or fail target decoding.
What changed
LinearProposalQueryinstead of emitting a per-token lifecycle event from local generation.finish_generation.Deadline and performance
commit_generationfor 250 ms behind a stricter 5 ms proposal deadline and verifies that decode returns without waiting for the callback to finish.Correctness coverage
Validation
cargo fmt --all -- --checkjust with-lld cargo check -p mesh-native-serving-plugin-api -p skippy-server -p mesh-native-serving-plugin-hostjust with-lld cargo clippy -p skippy-server -p mesh-native-serving-plugin-host --all-targets -- -D warningsjust with-lld cargo test -p mesh-native-serving-plugin-host --lib— 13 passedSummary by CodeRabbit
Improvements
Reliability