fix(plugin-host): fence passive proposal outcomes before finish - #1235
fix(plugin-host): fence passive proposal outcomes before finish#1235i386 wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughNative plugin finishing waits for queued passive work. Terminal reports and discards use reserved queue capacity. Failed native finishes retain generation state. Tests cover callback ordering, deadlines, and fence capacity. ChangesNative finish ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginDriver
participant PassiveWorker
participant NativePlugin
PluginDriver->>PassiveWorker: enqueue report or discard
PluginDriver->>PassiveWorker: enqueue terminal Fence
PassiveWorker-->>PluginDriver: acknowledge Fence
PluginDriver->>NativePlugin: invoke proposal callback
PluginDriver->>NativePlugin: invoke finish_generation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
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/mesh-native-serving-plugin-host/src/plugin_dispatch.rs`:
- Around line 318-335: Reserve one terminal queue slot exclusively for Fence:
update enqueue_terminal and the disposition enqueue path to allow Report/Discard
only up to the terminal capacity minus one, while the Fence path uses the full
PLUGIN_TERMINAL_RESERVE capacity. Preserve existing error handling and ensure
finish_after_passive_fence can enqueue its fence after dispositions fill their
reduced limit.
In `@crates/mesh-native-serving-plugin-host/src/plugin_dispatch/tests.rs`:
- Around line 267-272: The test currently bypasses the production dispatch path
by calling finish_after_passive_fence directly. Update the test around the
spawned finish thread to enqueue PluginCommand::Finish through PluginDriver,
then assert the fake plugin event log records "discard" before "finish",
exercising plugin_worker’s Finish handling.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afa27334-95f0-4c59-9391-f44d937b0cfd
📒 Files selected for processing (4)
crates/mesh-native-serving-plugin-host/src/lib.rscrates/mesh-native-serving-plugin-host/src/plugin_dispatch.rscrates/mesh-native-serving-plugin-host/src/plugin_dispatch/tests.rscrates/mesh-native-serving-plugin-host/src/test_support.rs
Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Summary
The native serving plugin host dispatches proposal
report/discardcallbacks on a passive worker, while generation lifecycle callbacks run on the primary worker. A passive outcome could therefore still be pending whenfinish_generationran, leaving Cacheline with an unresolved proposal and poisoning the next request.This PR:
finish_generation;The proposal deadline path remains independent of passive callback latency.
Verification
cargo fmt --all -- --checkcargo test -p mesh-native-serving-plugin-host(25 passed)This is intended to be cherry-picked into
general-inference/mesh-llm-privatemain after the public PR lands.Summary by CodeRabbit
Bug Fixes
Tests